-
Notifications
You must be signed in to change notification settings - Fork 557
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.
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
fix: iac unit tests to run as part of CI #1731
Conversation
c5dcbca
to
93f58d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rontalx I made a couple suggestions to fix the new unit tests to run
c010f56
to
371fe16
Compare
371fe16
to
cc9331a
Compare
@@ -164,7 +164,7 @@ describe('test --json-file-output ', () => { | |||
); | |||
|
|||
if (isWindows) { | |||
test( | |||
test.skip( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan to unskip this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hammer proposed to skip it for now, Tardis are working on fixing this flakey test:
https://snyk.slack.com/archives/C01D6U0KHGC/p1615982085024100
https://snyk.slack.com/archives/CFTAUCYQ7/p1615970075003300
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anthogez FYI, skipped it here based on @maxjeffos recommendation from the threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rontalx To clarify Tardis proposed to skip the test test/cli-json-file-output.spec.ts
that belongs to Hammer and they agreed. (Even if I do not like to skip/delete tests but it was necessary to move on)
Tardis work is done here because test/ecosystems-monitor-cpp.spec.ts
is not the root cause of the issue but this file that is currently flaking for windows and is under Hammer ownership.
By decoupling the /
character in unlinkSync(./${jsonOutputFilename}
); and giving it a generic handling that works for both Unix and Windows OS - should solve the flakiness for the 1st failed test
https://github.com/snyk/snyk/blob/master/test/cli-json-file-output.spec.ts#L84
cc @aron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, sry, it's a hammer tests, and @maxjeffos and I are aligned on it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but would like someone else to review the skip
call in test/cli-json-file-output.spec.ts
What does this PR do?
Aligns with the new jest unit-test folder structure & scripts that @maxjeffos has suggested based on:
https://github.com/snyk/snyk/tree/chore/separate-jest-unit-and-acceptance-tests
This PR also fixes lots of iac unit-tests that were apparently failing, but were actually not being run so it was missed.
Based on a discussion with @maxjeffos, you folks will rebase on top of this once I get this to master, in order for our iac-unit-tests to be passing successfully on your branch.
See thread for details:
https://snyk.slack.com/archives/C0127HWU0E7/p1615899204013500