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
Merged

Conversation

@mhdawson
Copy link
Contributor

@mhdawson mhdawson commented Nov 23, 2021

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

@kibertoad kibertoad commented Nov 23, 2021

Shouldn't this be documented somewhere?

Copy link
Member

@mcollina mcollina left a comment

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

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

@mcollina
Copy link
Member

@mcollina mcollina commented Nov 23, 2021

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 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

@mcollina mcollina commented Nov 23, 2021

A timeout error would point to that block of code.

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

@mhdawson mhdawson commented Nov 24, 2021

@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

lgtm

@mcollina mcollina merged commit 03dac4d into pinojs:master Nov 24, 2021
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants