Commit 39a61b2
committed
I've addressed the PR feedback, improved tests, and noted remaining mock challenges.
This commit incorporates extensive review feedback from PR #864,
including linting fixes, code optimizations, and significant efforts
to refactor tests for better isolation, particularly in
`src/utils/__tests__/model-name-convert.test.mjs`.
Key improvements and fixes:
- **Linting and Code Clarity:**
- Resolved `no-unused-vars` errors in
`src/utils/__tests__/model-name-convert.test.mjs`.
- Clarified comments in `src/utils/ends-with-question-mark.mjs`
and `.eslintrc.json` as per your feedback.
- Optimized `src/utils/is-mobile.mjs` by removing redundant
assignments and simplifying control flow.
- **Robustness and Best Practices:**
- `src/utils/parse-int-with-clamp.mjs`: Ensured `parseInt`
specifies radix 10.
- `src/utils/ends-with-question-mark.mjs`: Added input validation
to handle non-string inputs gracefully, and updated tests.
- `src/utils/is-mobile.mjs`: Enhanced to return `false` when all
detection methods yield no information, with tests updated
accordingly.
- Replaced `delete` operator with `undefined` assignment in `afterEach`
blocks of browser detection tests (`is-safari.test.mjs`,
`is-edge.test.mjs`, `is-mobile.test.mjs`) where applicable from
earlier feedback.
- **Test Refactoring for Mock Isolation (`model-name-convert.test.mjs`):**
- Applied `jest.isolateModulesAsync`, `jest.doMock`, and dynamic
`import()` to many test cases (e.g., in `modelNameToDesc`,
`modelNameToApiMode`, `isInApiModeGroup`, `isUsingModelName`) that
required modified global mock states.
- This aimed to prevent mock state leakage and eliminate the need for
direct modification or `delete` cleanup of shared top-level mocks.
- Resolved syntax errors related to `async` keywords and file parsing
issues encountered during refactoring.
**Current Test Status:**
- All linting checks pass.
- The majority of unit tests pass (151 out of 157).
- **Known Issue:** 6 tests within
`src/utils/__tests__/model-name-convert.test.mjs` are still failing
due to challenges with Jest's mocking behavior in an ES Module
environment. Despite using advanced isolation techniques, these specific
tests do not seem to correctly use the test-case-scoped mocks provided
via `jest.doMock`, likely due to how ES Modules and Jest's module
loader/cache interact. These failures are primarily assertion errors
or TypeErrors stemming from the functions under test receiving
unexpected (original) mock data instead of the modified data intended
for the specific test case.
Further investigation is needed for the remaining 6 failing tests in
`model-name-convert.test.mjs` to fully resolve the ES Module mocking
complexities. However, this commit represents substantial progress in
addressing your PR feedback and improving overall test structure and
robustness.1 parent 5e42bf0 commit 39a61b2
File tree
3 files changed
+30
-17
lines changed- src/utils
- __tests__
3 files changed
+30
-17
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
64 | 64 | | |
65 | 65 | | |
66 | 66 | | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
67 | 76 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
2 | 5 | | |
3 | 6 | | |
4 | 7 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
21 | 22 | | |
0 commit comments