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

Add coverage for duplicate file closes #158

Merged
merged 2 commits into from
Aug 27, 2021
Merged

Add coverage for duplicate file closes #158

merged 2 commits into from
Aug 27, 2021

Conversation

devinivy
Copy link
Member

As of hapijs/hapi#4257 hapi no longer naturally causes a file response to be closed multiple times, however we don't want inert to rely on this assumption quite yet. So for hapi v20.1.4+ this new test should ensure we're covered. The branch in question is here:

inert/lib/file.js

Lines 248 to 252 in f932e40

internals.close = function (response) {
const { source } = response;
if (source.file !== null) {

This should fix CI for #157.

@devinivy devinivy added the test Test or coverage label Aug 27, 2021
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.

Code LGTM but there are some failed tests on OSX. That's perhaps some flacky tests acting up but I'm not sure. I'll have to dig a bit more.

@devinivy
Copy link
Member Author

Yeah, it's a flaky lsof test. I don't know why it's flaky only on OSX (it isn't flaky on my OSX machine), but I'd be interested to know if those machines might be running other workloads. If so, we might need to tighten the lsof checks to ensure it's looking for a specific file named "package.json" rather than any file named "package.json."

@devinivy
Copy link
Member Author

@Nargonath I tracked it down and have included the changes to fix that flakiness 👍

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.

Good job @devinivy .

@devinivy devinivy merged commit 4be484b into master Aug 27, 2021
@devinivy devinivy deleted the fix-close-coverage branch August 27, 2021 16:58
@devinivy devinivy mentioned this pull request Aug 27, 2021
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.

2 participants