fix: Resolve Web Core CI failures (workflow, tests, and linting)#1001
fix: Resolve Web Core CI failures (workflow, tests, and linting)#1001jacobsimionato wants to merge 1 commit intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the test execution commands in package.json to target specific subdirectories and modifies types.test.ts to replace type-level variable assignments with runtime assertions to satisfy linting requirements. Feedback indicates that the shell glob expansion in the package scripts is not cross-platform and suggests using a globbing utility. Additionally, for type-level tests, it is recommended to use ESLint disable comments instead of adding unnecessary runtime assertions.
| }, | ||
| "test": { | ||
| "command": "node --test dist", | ||
| "command": "node --test dist/src/v0_8/*/*.test.js dist/src/v0_9/*/*.test.js dist/src/v0_9/*/*/*.test.js", |
There was a problem hiding this comment.
This change relies on shell glob expansion to find test files, which is not cross-platform. While this will work on Unix-like shells (Linux, macOS), it will likely fail on Windows cmd.exe because it does not expand wildcard (*) characters in command arguments. The node executable would receive the unexpanded string literal.
Additionally, the test paths are hardcoded and duplicated in the test:coverage script (line 67), creating a maintenance burden.
For a robust and cross-platform solution, consider using a helper package like glob to programmatically find the test files and pass them to node --test. This would ensure the test command works consistently across all developer environments.
There was a problem hiding this comment.
Wow, weren't these tests working before?
There was a problem hiding this comment.
Nup! I can't believe it either!
Yes, let's try the glob approach - that sounds better.
There was a problem hiding this comment.
hmmm...I have no trouble running all the tests by simply running
node --test dist
locally, no globbing patterns required. Do we need to do this? dist seems like a sufficient target and should work on windows.
| const inferredIsAny: IsAny<InferredType> = false; | ||
| assert.strictEqual(inferredIsAny, false); |
There was a problem hiding this comment.
While adding an assertion is a valid way to address the no-unused-vars linting error, it adds runtime code that always passes and doesn't contribute to the test logic itself. The primary check here is performed at compile-time by TypeScript, making the variable intentionally unused at runtime.
An alternative and often clearer approach for such type-level tests is to use an ESLint disable comment. This explicitly communicates the intent without adding boilerplate runtime code. This same feedback applies to the typesMatchExact variable on line 134.
Consider this alternative:
| const inferredIsAny: IsAny<InferredType> = false; | |
| assert.strictEqual(inferredIsAny, false); | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| const inferredIsAny: IsAny<InferredType> = false; |
There was a problem hiding this comment.
There's no need for these assertions, if the inference was broken, assigning to "false" on line 122 wouldn't compile, I think!
There was a problem hiding this comment.
Ahhh I see, that makes sense. Let's add the eslint disable comment instead.
ditman
left a comment
There was a problem hiding this comment.
I think you can revert the changes to types.test.ts (?)
| const inferredIsAny: IsAny<InferredType> = false; | ||
| assert.strictEqual(inferredIsAny, false); |
There was a problem hiding this comment.
There's no need for these assertions, if the inference was broken, assigning to "false" on line 122 wouldn't compile, I think!
| }, | ||
| "test": { | ||
| "command": "node --test dist", | ||
| "command": "node --test dist/src/v0_8/*/*.test.js dist/src/v0_9/*/*.test.js dist/src/v0_9/*/*/*.test.js", |
There was a problem hiding this comment.
Wow, weren't these tests working before?
| }, | ||
| "test": { | ||
| "command": "node --test dist", | ||
| "command": "node --test dist/src/v0_8/*/*.test.js dist/src/v0_9/*/*.test.js dist/src/v0_9/*/*/*.test.js", |
There was a problem hiding this comment.
hmmm...I have no trouble running all the tests by simply running
node --test dist
locally, no globbing patterns required. Do we need to do this? dist seems like a sufficient target and should work on windows.
|
I suppose I would also add that if we're worried about windows compilation, can we ask for a windows machine in a job? Most of us at Google are using POSIX-compliant systems (gLinux or macOS), so I don't think we're going to catch regressions like that on our own very well. |
Description
This PR addresses multiple issues preventing the
web_coretests from running successfully locally and passing in CI.Description of Changes
runs-on: ubuntu-latestproperty to thelintjob in.github/workflows/web_build_and_test.ymland standardized thepathsglob pattern torenderers/web_core/**.testandtest:coveragescripts inrenderers/web_core/package.jsonto use explicit single-level wildcard patterns (dist/src/v0_8/*/*.test.js dist/src/v0_9/*/*.test.js dist/src/v0_9/*/*/*.test.js) instead of simply pointing to thedistdirectory or using quoted**globs.@typescript-eslint/no-unused-varslinting errors inrenderers/web_core/src/v0_9/catalog/types.test.tsby asserting the unused type-level variables at runtime viaassert.strictEqual().Rationale
runs-onproperty in thelintjob caused GitHub Actions to throw a parsing error, marking the entireweb_build_and_test.ymlworkflow as invalid and preventing both tests and linting from running at all.node --test) can fail with module resolution (MODULE_NOT_FOUND) errors when pointed directly at a compiled directory likedistin a"type": "module"project. Node 20 environments (like the one used in GitHub Actions) do not internally expand quoted**globbing without shell assistance. Switching to explicit wildcard globs allows the files to be correctly resolved by the runner across local and CI environments.npm run lintCI job to fail. Adding assertions removes the violations.Testing/Running Instructions
renderers/web_coredirectory:cd renderers/web_corenpm run test(should pass successfully and find all tests).npm run test:coveragenpm run lint(should pass with no errors).Web coreworkflow now successfully parses, triggers, and completes both thebuild-and-testandlintjobs.Pre-launch Checklist