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

Switch back to Hoek.clone from lodash.deepClone. #898

Merged
merged 10 commits into from Dec 17, 2018
Merged

Conversation

@Invader444
Copy link
Contributor

Invader444 commented Nov 24, 2018

Fix lint-md script in package.json to work on Windows.

Fix lint-md script in package.json to work on Windows.
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 24, 2018

With this patch, you should have no issues with a set of Windows builds on Travis. Can we do that too as part of this PR? :)

@geek geek added this to the 18.0.1 milestone Nov 27, 2018
@geek

This comment has been minimized.

Copy link
Member

geek commented Nov 27, 2018

@Invader444 Awesome! Yes please update this PR with

os:
  - windows
  - linux
  - osx

for travis

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 27, 2018

Well.. that is bizarre! I have no such errors in my test output! Ill look into this more tomorrow :/

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Nov 27, 2018

Classic \n -> \r\n on windows 🤦‍♂

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 27, 2018

Ohhh. Git needs to checkout without auto crlf?

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Nov 27, 2018

We rather need to test with regexps matching both cases.

.travis.yml Show resolved Hide resolved
Invader444 and others added 5 commits Nov 27, 2018
Trying to force eol=lf
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Dec 4, 2018

lol!

lib/reporters/console.js missing coverage on line(s): 409-411, 413, 416-418, 420
Code coverage below threshold: 99.65 < 100

::sigh::

I'm not sure why I don't see these Windows errors, although, for one thing I am getting unix-style newlines in my output... perhaps because of nvm?

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Dec 6, 2018

@geek, does that coverage report make sense to you? It looks like the line numbers may be inaccurate... :(

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 8, 2018

@Invader444 that missing coverage appears to be related to the supports color tty detection. We are using an outdated version and may just need to update to the latest.

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Dec 16, 2018

With latest commit:

C:\Users\Jon\workspace\git\Doomtrooper\lab>npm run test
npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/
npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/

> lab@18.0.0 test C:\Users\Jon\workspace\git\Doomtrooper\lab
> node ./bin/_lab -fL -t 100 -m 3000



  ..................................................
  ..................................................
  ..................................................
  ..................

  .

1 tests complete
Test duration: 1 ms
No global variable leaks detected

................................
  ..................................................
  ..................................................
  ................................................

348 tests complete
Test duration: 42994 ms
No global variable leaks detected
Coverage: 100.00%
Linting results: No issues


> lab@18.0.0 posttest C:\Users\Jon\workspace\git\Doomtrooper\lab
> npm run lint-md

npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/
npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/

> lab@18.0.0 lint-md C:\Users\Jon\workspace\git\Doomtrooper\lab
> eslint --config hapi --rule "strict: 0, eol-last: 0" --plugin markdown --ext md --parser-options "ecmaVersion: 9" .


C:\Users\Jon\workspace\git\Doomtrooper\lab>
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Dec 16, 2018

lib/reporters/console.js missing coverage on line(s): 409-411, 413, 416-418, 420
Code coverage below threshold: 99.65 < 100

@geek any other ideas? :(

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 16, 2018

@Invader444 thoughts on removing windows from travis until we can sort out the build issue? If we revert the travis change then I'll go ahead and merge/release and we can come back to build issues.

@geek geek self-assigned this Dec 16, 2018
Use rimraf.sync instead of fs.rmdirSync to delete directories in tests, in order to prevent spurious ENOENT errors on Windows.
@Invader444 Invader444 force-pushed the secretcowlevel:master branch 3 times, most recently from 9829d3c to 47fa336 Dec 16, 2018
Coverage should not be dependent on the build environment's console, and Travis CI seems to set process.stdout.isTTY to false on its Windows environment.
@Invader444 Invader444 force-pushed the secretcowlevel:master branch from 47fa336 to a9c6b7a Dec 16, 2018
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Dec 16, 2018

Eureka! A few more tweaks were needed, but it seems to be working now :)

process.stdout.isTTY is false for windows travis builds... they've done this before too: chalk/supports-color#63

Ideally this would be fixed in supports-color upstream, but looks like they won't do that as the last time they tried in the above PR it broke backwards compatibility for some of their package's consumers.

@geek
geek approved these changes Dec 17, 2018
@geek geek merged commit 5c11479 into hapijs:master Dec 17, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 17, 2018

@Invader444 thanks a lot for your help. This is now published as 18.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.