Skip to content

fix: Resolve Web Core CI failures (workflow, tests, and linting) #1001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jacobsimionato wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −6
Open

fix: Resolve Web Core CI failures (workflow, tests, and linting) #1001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/workflows/web_build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ on:
push:
branches: [ main ]
paths:
- 'renderers/web_core/**/*'
- 'renderers/web_core/**'
- '.github/workflows/web_build_and_test.yml'
pull_request:
paths:
- 'renderers/web_core/**/*'
- 'renderers/web_core/**'
- '.github/workflows/web_build_and_test.yml'

jobs:
Expand All @@ -49,6 +49,8 @@ jobs:
working-directory: ./renderers/web_core
run: npm run test
lint:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v6

Expand Down
4 changes: 2 additions & 2 deletions renderers/web_core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
"build": "wireit",
"build:tsc": "wireit",
"test": "wireit",
"test:coverage": "c8 node --test \"dist/**/*.test.js\"",
"test:coverage": "c8 node --test dist/src/v0_8/*/*.test.js dist/src/v0_9/*/*.test.js dist/src/v0_9/*/*/*.test.js",
"compile": "tsc",
"lint": "gts lint",
"fix": "gts fix"
Expand Down Expand Up @@ -104,7 +104,7 @@
"clean": "if-file-deleted"
},
"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",
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Copy link
Copy Markdown
Collaborator

@ditman ditman Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, weren't these tests working before?

Copy link
Copy Markdown
Collaborator Author

@jacobsimionato jacobsimionato Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nup! I can't believe it either!

Yes, let's try the glob approach - that sounds better.

Copy link
Copy Markdown
Collaborator

@ava-cassiopeia ava-cassiopeia Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"dependencies": [
"build"
]
Expand Down
6 changes: 4 additions & 2 deletions renderers/web_core/src/v0_9/catalog/types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ describe('InferredComponentApiSchemaType', () => {
name: 'MockComp',
schema: mockSchema,
} satisfies ComponentApi;
// Appease typescript-eslint/no-unused-vars
type _ = typeof mockApi;

assert.ok(mockApi);

// Type-level equivalence assertion using z.infer
type ExpectedType = z.infer<typeof mockSchema>;
Expand All @@ -120,6 +120,7 @@ describe('InferredComponentApiSchemaType', () => {
// This happens when `mockApi: ComponentApi`, but doesn't when
// `mockApi {} satisfies ComponentApi`!
const inferredIsAny: IsAny<InferredType> = false;
assert.strictEqual(inferredIsAny, false);
Comment on lines 122 to +123
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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:

Suggested change
const inferredIsAny: IsAny<InferredType> = false;
assert.strictEqual(inferredIsAny, false);
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const inferredIsAny: IsAny<InferredType> = false;

Copy link
Copy Markdown
Collaborator

@ditman ditman Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for these assertions, if the inference was broken, assigning to "false" on line 122 wouldn't compile, I think!

Copy link
Copy Markdown
Collaborator Author

@jacobsimionato jacobsimionato Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh I see, that makes sense. Let's add the eslint disable comment instead.


// When types are not "any", check that they're the same by checking if they
// extend each other.
Expand All @@ -132,5 +133,6 @@ describe('InferredComponentApiSchemaType', () => {
// typesMatchExact only accepts "true" if `TypesAreEquivalent`
const typesMatchExact: TypesAreEquivalent<ExpectedType, InferredType> =
true;
assert.strictEqual(typesMatchExact, true);
});
});
Loading