Skip to content

Tests: Run slow scanArtifactsForTokens test in CI only by default #3874

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

Merged
henrymercer merged 2 commits into from
May 7, 2026
Merged

Tests: Run slow scanArtifactsForTokens test in CI only by default #3874

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c9cd77
Tests: Run slow `scanArtifactsForTokens` test in CI only by default
henrymercer May 6, 2026
0c80cee
Add explicit error on Windows
henrymercer May 7, 2026
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
7 changes: 6 additions & 1 deletion src/artifact-scanner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,12 @@ test("scanArtifactsForTokens handles files without tokens", async (t) => {
}
});

if (os.platform() !== "win32") {
// This test is slow (extracts and scans a zip artifact), so by default we only run it in CI. Set
// RUN_SLOW_TESTS=1 to run it locally.
if (
os.platform() !== "win32" &&

@mbg mbg May 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we are only running this test in CI, why not also run it on Windows? Given the nature of the function being tested, it seems like we could easily miss a Windows-specific problem.

If there is a good reason why we aren't running it on Windows, it would be good if the comment noted it and explained why.

@henrymercer henrymercer May 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The artifact scanner isn't compatible with Windows — it's only used in our debug artifact tests, which always run on Linux. It's not compatible because it directly shells out to extract archives.

@mbg mbg May 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I had a look at the implementation but didn't see an obvious guard against running on Windows. I saw the isInTestMode() check, but I didn't verify whether we always run on Linux there.

In that case, it might be good to make this more explicit in the main implementation in scanArtifactsForTokens as well and return with a suitable message there if it is somehow running on Windows?

@henrymercer henrymercer May 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, done!

(process.env.CI === "true" || process.env.RUN_SLOW_TESTS === "1")
) {
test("scanArtifactsForTokens finds token in debug artifacts", async (t) => {
t.timeout(15000); // 15 seconds
const messages: LoggedMessage[] = [];
Expand Down
Loading