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 tests for citgm #4221

Merged
merged 5 commits into from
Jan 22, 2021
Merged

Fix tests for citgm #4221

merged 5 commits into from
Jan 22, 2021

Conversation

devinivy
Copy link
Member

There are a few issues to fix for failures in citgm:

  1. Tests that rely on lsof on node v14. @cjihrig previously made a fix for this in allow tests using lsof to pass on ENOENT #3744, but on node v14 the way to detect this error changed. lookup: add hapi nodejs/citgm#436 (comment)
  2. Some flaky/failing tests on macos and AIX. As far as I can tell the issue here is flakiness due to timing-dependent tests and slow IO in CI. I attempted to make these tests less timing-dependent and to better cleanup after themselves, which allows them to be retryable. I would love some review of these changes, as they are some of the more complex tests in the hapi test suite. lookup: add hapi nodejs/citgm#436 (comment)

CC @AdriVanHoudt I know in nodejs/citgm#436 you were able to reproduce the macos issues. If you have time and would be open to trying out these changes it would be very much appreciated.

@devinivy devinivy added the test Test or coverage label Jan 19, 2021

req.on('error', (err) => {

expect(err).to.not.exist();
Copy link
Member Author

Choose a reason for hiding this comment

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

In some (flaky) cases this error was being triggered late and causing the next test to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lab does not bind the expect to a specific test, so it can only assume that errors are caused by the current test.

@devinivy devinivy mentioned this pull request Jan 19, 2021
1 task
Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

Mostly nits, but the "handles race condition between equal client and server timeouts" rewrite seems like it is not doing the same thing.

test/core.js Outdated Show resolved Hide resolved

req.on('error', (err) => {

expect(err).to.not.exist();
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lab does not bind the expect to a specific test, so it can only assume that errors are caused by the current test.

test/request.js Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
test/request.js Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
@devinivy
Copy link
Member Author

devinivy commented Jan 21, 2021

@kanongil I appreciate the review, and I've incorporated your feedback.

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

Still a few nits.

test/request.js Outdated Show resolved Hide resolved
test/request.js Outdated Show resolved Hide resolved
@devinivy devinivy merged commit 7e99521 into master Jan 22, 2021
@devinivy devinivy deleted the citgm-fixes branch January 22, 2021 14:28
@devinivy devinivy self-assigned this Jan 22, 2021
@devinivy
Copy link
Member Author

I appreciate all the careful review 👍

@devinivy devinivy added this to the 20.1.0 milestone Jan 25, 2021
@AdriVanHoudt
Copy link
Contributor

For what it's worth I still see these on master

Failed tests:

  393) Request active() exits handler early when request is no longer active:

      Timed out (5000ms) - Request active() exits handler early when request is no longer active


  417) Request _postCycle() skips onPreResponse when validation terminates request:

      Timed out (5000ms) - Request _postCycle() skips onPreResponse when validation terminates request

I don't have the bandwidth to dive into this atm but if I can help by running some tests or something let me know. (for quicker response dm me on slack)

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

Successfully merging this pull request may close these issues.

4 participants