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

Increase timeout for mocha to 7500 #4887

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Increase timeout for mocha to 7500 #4887

merged 1 commit into from
Apr 21, 2022

Conversation

grisu48
Copy link
Contributor

@grisu48 grisu48 commented Apr 8, 2022

In order to fix some timeout issues in the CI on low-end hardware we increase
the timeout for mocha to 7500ms.

This is a proposed fix for #4886

@dougwilson
Copy link
Contributor

Thanks @grisu48 ! The particular test referenced in the issue can take a bit due to the memory allocation for it. Is that the only test that times out? If so, I can update your PR to isolate the expanded test timeout only to that single test.

@grisu48
Copy link
Contributor Author

grisu48 commented Apr 9, 2022

Yes, the rest of the CI works nicely with this timeout. With the here provided fix, the following test runs completes without any errors:

npm config set shrinkwrap false && npm rm --silent --save-dev connect-redis && npm run test-ci

The should not stack overflow with many registered routes test clocks in at ~3700ms, therefore we doubled this duration and came up with the suggested 7 seconds.

Important: Since the CI test runs cancels after the first failure, I cannot comment if other tests are not affected yet. Will look on Monday at the logs and try to determine if other tests are affected as well.

test/Router.js Outdated Show resolved Hide resolved
@dougwilson dougwilson changed the base branch from master to 4.18 April 14, 2022 03:32
@grisu48 grisu48 requested a review from dougwilson April 14, 2022 11:54
@grisu48
Copy link
Contributor Author

grisu48 commented Apr 14, 2022

@dougwilson I adapted your solution and tested in on my affected aarch64 VM, where 5/5 test runs are now passing. Please review again, IMHO this would solve the issue.

@dougwilson dougwilson merged commit fd8e45c into expressjs:4.18 Apr 21, 2022
@grisu48 grisu48 deleted the timeout branch April 21, 2022 11:29
@grisu48
Copy link
Contributor Author

grisu48 commented Apr 21, 2022

Thank you for merging! 🙂 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants