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: make test timeouts configurable #1238

Merged
merged 3 commits into from Nov 24, 2021

Conversation

mhdawson
Copy link
Contributor

We saw some timeouts in the tests when we were running to validate in Red Hat containers (for example: https://catalog.redhat.com/software/containers/ubi8/nodejs-12/5d3fff015a13461f5fb8635a).

Our investigation so far points to it being loading/resource issue on the machines we are running versus anything in the tests.

We'd like to be able to extended timeouts in the tests, this PR adds a simple way to do that through the environment.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@kibertoad
Copy link
Contributor

Shouldn't this be documented somewhere?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'd recommend we increase the default to 10s.

test/helper.js Outdated Show resolved Hide resolved
test/helper.js Outdated Show resolved Hide resolved
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me with suggestions applied. Will probably solve some flakiness in our own CI.

@mcollina
Copy link
Member

I'm not sure we should document this.. but if we want to
the place is https://github.com/pinojs/pino/blob/master/CONTRIBUTING.md.

@kibertoad
Copy link
Contributor

kibertoad commented Nov 23, 2021

I'm not sure we should document this

How would people know it's there without randomly stumbling upon it in code then?

@mcollina
Copy link
Member

A timeout error would point to that block of code.

mhdawson and others added 2 commits November 24, 2021 09:26
Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
@mhdawson
Copy link
Contributor Author

@mcollina thanks, accepted your suggestions.

From the output I'll confirm it was pretty easy to see that the failures were a timeout issue and where the timeouts where.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 03dac4d into pinojs:master Nov 24, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants