Skip to content
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

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Conversation

rontalx
Copy link
Contributor

@rontalx rontalx commented Mar 16, 2021

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

@rontalx rontalx force-pushed the fix/jest-config-not-provided branch from c5dcbca to 93f58d5 Compare March 16, 2021 18:19
jest.config.js Show resolved Hide resolved
@rontalx rontalx requested a review from maxjeffos March 16, 2021 19:41
Copy link
Contributor

@maxjeffos maxjeffos left a 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

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@rontalx rontalx force-pushed the fix/jest-config-not-provided branch from c010f56 to 371fe16 Compare March 17, 2021 08:56
@rontalx rontalx changed the title fix: jest config not provided fix: iac unit tests to run as part of CI Mar 17, 2021
@rontalx rontalx force-pushed the fix/jest-config-not-provided branch from 371fe16 to cc9331a Compare March 17, 2021 11:30
@rontalx rontalx requested a review from maxjeffos March 17, 2021 11:30
@snyk snyk deleted a comment from github-actions bot Mar 17, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2021

Expected release notes (by @rontalx)

fixes:
temporarly skip flaky test (453db4c)
jest skipping iac unit-tests + fixed failing tests (cc9331a)

others (will not be included in Semantic-Release notes):
add new test folder to codeowners (bd0adfb)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@rontalx rontalx marked this pull request as ready for review March 17, 2021 13:25
@rontalx rontalx requested a review from a team as a code owner March 17, 2021 13:25
@rontalx rontalx requested a review from a team March 17, 2021 13:25
@rontalx rontalx requested a review from a team as a code owner March 17, 2021 13:25
@rontalx rontalx requested a review from aron March 17, 2021 13:26
@@ -164,7 +164,7 @@ describe('test --json-file-output ', () => {
);

if (isWindows) {
test(
test.skip(
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

@anthogez anthogez Mar 17, 2021

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

Copy link
Contributor Author

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks all 👍

Copy link
Contributor

@aron aron left a 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

This was referenced Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants