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.

Sign up for GitHub

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

Jump to bottom

add Node.js v10 to matrix #3350

Merged
merged 14 commits into from
Apr 27, 2018
Merged

add Node.js v10 to matrix #3350

merged 14 commits into from
Apr 27, 2018

Conversation

boneskull
Copy link
Contributor

AFAIK Node.js v9.x is no longer maintained as of today.

@boneskull
drop Node.js v9 support in lieu of Node.js v10
db1cc36 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull boneskull added type: chore generally involving deps, tooling, configuration, etc. qa semver-major implementation requires increase of "major" version number; "breaking changes" labels Apr 24, 2018
@boneskull
fix npm version to 5.x
fdc2c51 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
fix npm version to 5.x in AppVeyor
4b5b8e5 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
Bamieh
Bamieh suggested changes Apr 25, 2018
View reviewed changes
Copy link
Contributor

@Bamieh Bamieh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Node.js 9 ends life in June 2018. It will no longer receive maintenance starting from 2018-04-01 But I believe we should keep using it until June.

If you disagree about keeping it feel free to dismiss the review.

@plroebuck
Copy link
Contributor

@Bamieh wasn't Node9's raison d'être the development/EA release of Node10? If Node10 is available, why bother?

@plroebuck
Copy link
Contributor

This was kinda humorous though...

@plroebuck
Copy link
Contributor

plroebuck commented Apr 25, 2018 •
edited
Loading

Maybe consider using a single fake environment variable (e.g., MOCHA_TEST) in ".travis.yaml" for each job to annotate what/why. For instance, the last job appears to be running browser tests where others are testing Node. So maybe the last job has MOCHA_TEST=bundle,browser and the others MOCHA_TEST=node. Maybe the COVERAGE variable could be in the environment as well. Smoke tests could annotate MOCHA_TEST=production. Just a thought.

@boneskull
Copy link
Contributor Author

@Bamieh Noted; I'm not sure what "end of life" vs. "not maintained" means, exactly. I'll try to clarify with the release team.

@boneskull
Copy link
Contributor Author

@plroebuck We used to do this, but the reason we no longer do so is because it breaks caching (see my blog post about it). We run 3 builds using the same pre-cached dependency trees: linting, Node.js v, and finally browser tests. If, say, the browser test had an environment variable that the other two did not, we wouldn't gain the speed benefit of the pre-cached dependencies.

The fact that the builds look the is simply due to poor UX. Each build should allow for a unique name, but no such thing exists. Environment variables are displayed (because they want to display "something"), but those are functional and have other implications. The differentiator between our builds is the script, but the UI doesn't display it (for good reason).

We were kicking around the idea of migrating the build to Jenkins, and I'm still up for it, since that'd give us more control. Just don't have time to configure it. The server would be paid for by the JS Foundation.

@boneskull
Copy link
Contributor Author

OK, I have confirmation that Node.js v9 will receive security updates through June.

At that time, we should drop Node.js v9.

We can still add Node.js v10, so I'll modify this PR.

@boneskull
instead, don't drop Node.js v9.x
e256a7a 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
add smoke test for Node.js v9
086570f 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull boneskull added semver-patch implementation requires increase of "patch" version number; "bug fixes" and removed semver-major implementation requires increase of "major" version number; "breaking changes" labels Apr 25, 2018
@plroebuck
Copy link
Contributor

BTW, believe this line in your patch should be fast-tracked (.travis.yaml#L16) to master.

     cd ~/npm && npm install npm@^5

npm-6 dropped support for Node-4, so all submitted PRs will fail TravisCI on Node-4 test.

@boneskull
Copy link
Contributor Author

I'd fast-track it if appveyor didn't start breaking

@boneskull boneskull changed the title drop Node.js v9 support in lieu of Node.js v10 add Node.js v10 to matrix Apr 26, 2018
@plroebuck
Copy link
Contributor

Was about to look into that failure myself. It started failing symlink tests, and also seemed to have not installed Windows growl-substitute runtime dependency.

@boneskull
AppVeyor: add Node.js v10; use npm ci for speed
c2ab039 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
Copy link
Contributor Author

since AppVeyor doesn't seem to just build against master, it's maybe the changes to package-lock.json. I'm not sure they're backwards compatible (potentially npm bug). I'll see what happens when I revert a7b1139

@boneskull
Copy link
Contributor Author

adding [ci skip] to the commit message when modifying package-lock.json is a mistake which I won't repeat

@boneskull
revert package-lock.json to what npm@5 uses
79ae1be 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@plroebuck
Copy link
Contributor

Here's the link for Windows Growl:
http://www.growlforwindows.com/gfw/d.ashx?f=growlnotify.zip

@boneskull
Copy link
Contributor Author

It's hard to tell, but I'm not confident growl is the problem.

@boneskull
Copy link
Contributor Author

I'll try just ignoring npm...

@boneskull
don't install npm
33e9fc5 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
Copy link
Contributor Author

(the growl notification happens after the failure)

@boneskull
don't use npm ci since it doesn't necessarily exist
ab63ff5 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@outsideris outsideris mentioned this pull request Apr 26, 2018
Merged
@boneskull
Copy link
Contributor Author

I wonder if AppVeyor changed something on their side. those two failing tests should be skipped.

@boneskull
use npm@5 / ci but don't fast-finish
920e9a5 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
Copy link
Contributor Author

I'm just gonna pull this up on my windows box.

@boneskull
Copy link
Contributor Author

alright, I investigated this in windows, and while I found some other issues related to the dev environment, I couldn't reproduce this problem. I'm just going to change the logic to "skip if windows"--instead of feature-checking--and call it good

@boneskull
avoid all symlink tests on win32
95db7b8 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
ensure fast_finish is true
2a6d210 
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull
Copy link
Contributor Author

alright, LGTM.

@boneskull boneskull merged commit 1224a48 into master Apr 27, 2018
@boneskull boneskull deleted the node-10 branch April 27, 2018 21:59
@plroebuck
Copy link
Contributor

To ease anyone's OCD sensibilities when viewing Travis CI's build page, maybe these two lines:

mocha/.travis.yml

Line 35 in e256a7a

node_js: '8'

mocha/.travis.yml

Line 38 in e256a7a

node_js: '9'

could be swapped so the jobs run in descending order?

@boneskull
Copy link
Contributor Author

@plroebuck 08dbaa3

@boneskull boneskull added this to the next milestone May 17, 2018
boneskull added a commit that referenced this pull request May 18, 2018
@boneskull
add Node.js v10 to build; fix win32 issues (#3350)
d87b12e 
* fix npm version to 5.x
* AppVeyor: add Node.js v10; use npm ci for speed
* revert package-lock.json to what npm@5 uses
* add .gitattributes
* remove linebreak-style from eslintrc
* avoid all symlink tests on win32

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@plroebuck plroebuck mentioned this pull request Nov 25, 2018
Merged
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
@boneskull
add Node.js v10 to build; fix win32 issues (mochajs#3350)
4a80cf7 
* fix npm version to 5.x
* AppVeyor: add Node.js v10; use npm ci for speed
* revert package-lock.json to what npm@5 uses
* add .gitattributes
* remove linebreak-style from eslintrc
* avoid all symlink tests on win32

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@outsideris outsideris outsideris approved these changes

@Bamieh Bamieh Bamieh approved these changes

Assignees
No one assigned
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: chore generally involving deps, tooling, configuration, etc.
Projects
None yet
Milestone
v5.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@boneskull @plroebuck @outsideris @Bamieh