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

test: increase analytics test timeout #2157

Merged
3 commits merged into from
Aug 12, 2021
Merged

test: increase analytics test timeout #2157

3 commits merged into from
Aug 12, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 11, 2021

Launching process alone takes 5 seconds which is the default timeout. Causes unreliable tests.

https://app.circleci.com/pipelines/github/snyk/snyk/6706/workflows/1ae30a6e-a106-4d71-a337-dffd58512a09/jobs/44733

Also made some small fixes (see commits).

@ghost ghost requested review from a team as code owners August 11, 2021 18:32
@ghost ghost requested a review from jan-stehlik August 11, 2021 18:32
@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against 8e296a6

Jahed Ahmed added 2 commits August 11, 2021 18:39
This spy is never called as we're executing code outside the process.
Launching process alone takes 5 seconds which is the default timeout.
@ghost ghost force-pushed the test/analytics-fix branch from cb97952 to 2735bce Compare August 11, 2021 18:40
@ghost ghost force-pushed the test/analytics-fix branch from 2735bce to 8e296a6 Compare August 11, 2021 18:46
@@ -27,7 +30,7 @@ describe('analytics module', () => {
});

afterEach(() => {
jest.restoreAllMocks();
server.clearRequests();
Copy link
Author

Choose a reason for hiding this comment

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

Added this as we should typically clear state between tests so we aren't asserting on invalid state.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

import * as request from '../../../src/lib/request';
import * as fs from 'fs';

jest.setTimeout(1000 * 30);
Copy link
Author

Choose a reason for hiding this comment

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

This is the main thing to avoid test timeouts.

Comment on lines +311 to +313

const lastRequest = server.popRequest();
expect(lastRequest).toBeUndefined();
Copy link
Author

Choose a reason for hiding this comment

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

Spying won't work as spies are bound to the current Node context; which is Jest. We are launching Snyk CLI in its own process where the spy does not exist.

To test this correctly, we can ensure fakeServer didn't receive a request.

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.

Nice

@ghost ghost merged commit 01e348d into master Aug 12, 2021
@ghost ghost deleted the test/analytics-fix branch August 12, 2021 11:05
This pull request was closed.
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.

1 participant