Repository navigation
feat: react testing guide - #830
dennisvankekem wants to merge 16 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive React Testing Guide to the APL Console documentation to standardize how contributors write and review React tests (with emphasis on user-facing behavior, RTL query patterns, mocking boundaries, and async flows).
Changes:
- Added a new
docs/react-testing-guide.mdcovering recommended test goals, query priorities, mocking patterns, and common pitfalls. - Included concrete examples for RTK Query mutation mocking, React Router navigation mocking, and React Hook Form testing.
- Added a reviewer checklist and guidance on writing resilient, behavior-focused tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/react-testing-guide.md:1152
- This “Split by behaviour” snippet is tagged as TSX and uses
it("...");without callbacks. That’s invalid Jest syntax and could confuse readers; consider presenting these as plain text (or fullit("...", () => {})examples).
```tsx
it("populates the form from current settings");
it("trims node selectors before submitting");
it("removes empty node selectors");
it("submits null for an empty pull secret");
docs/react-testing-guide.md:326
- The “Good/Weak test names” snippets are marked as TSX and use
it("...");without a callback, which is invalid Jest syntax and easy to misread as copy/pastable code. Present these as plain text (or validit("...", () => {})examples) so the guide doesn’t model invalid patterns.
This issue also appears on line 1148 of the same file.
```tsx
it("disables saving while settings are being fetched");
it("preserves hidden Otomi settings when saving");
it("removes empty node selectors from the payload");
it("navigates to the settings overview after a successful save");
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/react-testing-guide.md:464
- Sentence starts mid-thought and reads like a fragment after the code block; making it a standalone sentence improves readability (and matches the guide’s general-present tone).
did not mean the test contained invalid syntax.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/react-testing-guide.md:464
- This sentence fragment starts with a lowercase verb and lacks a subject, which reads like a typo in the guide. It should be a complete sentence (e.g., starting with “This error …”).
did not mean the test contained invalid syntax.
docs/react-testing-guide.md:60
- These fenced blocks are marked as TypeScript (```ts) but the content is just a list of field names with semicolons. That reads like misleading TS and the syntax highlighting is wrong. Consider switching to a plain-text fence and omitting the semicolons, or show a real TS type/interface snippet.
```ts
version;
hasExternalDNS;
hasExternalIDP;
globalPullSecret;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/react-testing-guide.md:464
- Sentence starts mid-thought and with a lowercase letter, which reads like a fragment after the preceding code block. Consider rewriting it as a complete sentence with a subject.
did not mean the test contained invalid syntax.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/react-testing-guide.md:464
- This sentence starts mid-thought and reads like it’s referring to a past event. If the intent is to explain the meaning of the error generally, rephrase it as a complete sentence (and capitalize it).
did not mean the test contained invalid syntax.
docs/react-testing-guide.md:3
- The guide introduces a new
npm run coveragescript in package.json, but the doc doesn’t currently mention how to run tests/coverage locally. Adding a short “Running tests locally” section near the top would make the guide immediately actionable.
# React Testing Guide for the APL Console
## 1. Why we test React code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/react-testing-guide.md:53
- This fenced block is tagged as
ts, but the contents are not valid TypeScript (just field names). Consider using atextfence (or writing a valid TS type/interface) to avoid misleading syntax highlighting.
```ts
docs/react-testing-guide.md:464
- This line is a sentence fragment and reads as though it’s missing a subject (and it’s phrased like a one-time past event). Reword to a complete, general statement.
did not mean the test contained invalid syntax.
docs/react-testing-guide.md:43
- The fenced block is tagged as
ts, but the contents are just field names and not valid TypeScript. This is likely to confuse readers and produces misleading syntax highlighting; use atextfence (or a valid TS type snippet) instead.
This issue also appears on line 53 of the same file.
```ts
There was a problem hiding this comment.
🟢 Approval recommended
Changes are low-risk (docs + a script alias) and no functional/runtime behavior is modified.
Review details
Suppressed comments (1)
docs/react-testing-guide.md:464
- The sentence after the error snippet uses past tense and reads like a historical incident; since this section describes a general rule, it should be in present tense (and include the implied subject from the preceding line).
did not mean the test contained invalid syntax.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are additive and low-risk (docs + a script alias), with only a minor wording fix suggested in the new guide.
Review details
Suppressed comments (1)
docs/react-testing-guide.md:464
- This line is a sentence fragment (lowercase and missing a subject) in a general guidance section; rewrite it as a full sentence for clarity.
did not mean the test contained invalid syntax.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # package.json Co-authored-by: dennisvankekem <38350840+dennisvankekem@users.noreply.github.com>
Merged the latest |
There was a problem hiding this comment.
🟡 Changes recommended
The guide contains unresolved accuracy and testing-pattern issues, including incorrect file-upload and query-priority guidance and an unhandled rejected promise in the failure-path example.
3 open issues
1 resolved since last review
Review effort: Lite
| it("does not navigate when saving fails", async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| mockUnwrap.mockRejectedValue(new Error("Save failed")); |
| ```tsx | ||
| getByRole; | ||
| getByLabelText; | ||
| getByText; |
| fireEvent.change(fileInput, { | ||
| target: { files: [file] }, | ||
| }); |
There was a problem hiding this comment.
🔵 Needs a closer look
The guide contains two active documentation-test issues: one rejected-save example can cause an unhandled promise rejection, and the file-upload example bypasses the repository’s documented user-level interaction pattern.
3 open issues
Review effort: Lite
Considerations