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

fixes logic for lab.before timeout #652 #653

Merged
merged 2 commits into from Oct 28, 2016

Conversation

@kgraves
Copy link
Contributor

kgraves commented Oct 26, 2016

Here's my first stab at fixing the logic. (#652) I didn't see anything in the Hapi style guide on ternary operators, so I left it on a single line.

It seems to work fine, but for some reason one test is failing on me, even when I have a fresh repo without my changes:

Failed tests:

  65) Coverage identifies lines with partial coverage when having inline sourcemap:

      Expected [ { filename: 'test/coverage/while.js',
    lineNumber: '5',
    originalLineNumber: 11 },
  { filename: 'test/coverage/while.js',
    lineNumber: '6',
    originalLineNumber: 12 } ] to include [ { filename: './while.js',
    lineNumber: '5',
    originalLineNumber: 11 },
  { filename: './while.js',
    lineNumber: '6',
    originalLineNumber: 12 } ]

      at it (/home/kg/Documents/gits/lab/test/coverage.js:122:32


1 of 296 tests failed
Test duration: 35270 ms
No global variable leaks detected
Coverage: 100.00%
Linting results: No issues

I'm assuming it's not caused by my changes, however Travis-ci doesn't seem to be failing...
Any ideas?

@kgraves

This comment has been minimized.

Copy link
Contributor Author

kgraves commented Oct 27, 2016

It looks like it MAY have to do with source-map-support introducing some issues between 0.4.2 and 0.4.5 (current). see here

The last build of Lab on Travis-ci used 0.4.2 (see here), which was about a month ago.

Lab's package.json specifies 0.4.x.
I just ran the tests locally with source-map-support pinned at 0.4.2 and all tests pass.

Any ideas how to proceed? should I pin source-map-support at 0.4.2?

@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 27, 2016

@kgraves your change looks good. If you can add a test for the non-integer case then that will help with the coverage. I will address the other issue with travis and 0.4.5.

@kgraves

This comment has been minimized.

Copy link
Contributor Author

kgraves commented Oct 28, 2016

@geek I've added a unit test. Let me know if there's anything else missing. 🍺

@geek geek added the bug label Oct 28, 2016
@geek geek added this to the 11.2.0 milestone Oct 28, 2016
@geek geek self-assigned this Oct 28, 2016
@geek geek merged commit 1a944ac into hapijs:master Oct 28, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Node Security No known vulnerabilities found
Details
@aalimovs aalimovs mentioned this pull request Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.