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

test(ci): add early supported node versions to test matrix #705

Merged
merged 5 commits into from
Dec 6, 2018
Merged

test(ci): add early supported node versions to test matrix #705

merged 5 commits into from
Dec 6, 2018

Conversation

watson
Copy link
Contributor

@watson watson commented Dec 3, 2018 •
edited
Loading

We had an issue where an early version of Node.js v6 didn't have support for a core API we were using. This wasn't caught by our tests because our tests ran on the latest 6.x version.

By adding the first version in each major version range we support to the test matrix, we enhance our ability to catch these issues in the future.

@watson watson self-assigned this Dec 3, 2018
Qard
Qard previously approved these changes Dec 4, 2018
View reviewed changes
@watson
Copy link
Contributor Author

watson commented Dec 5, 2018

@Qard As we discussed yesterday, do you want me to remove 11.0.0, 10.0.0 and 8.0.0 so we only run these tests on 6.0.0?

@Qard
Copy link
Contributor

Qard commented Dec 5, 2018

I'm good either way.

@watson watson mentioned this pull request Dec 5, 2018
Merged
@watson
Copy link
Contributor Author

watson commented Dec 5, 2018 •
edited
Loading

I should have checked before this last commit - there's no patch releases on the first release of each major version. So this doesn't change a thing 😂

I was just wondering if the failures on Node.js 8.0.0 could be "fixed" by using the newest 8.0.x

@watson
Copy link
Contributor Author

watson commented Dec 6, 2018

@Qard the bug I fixed in test/instrumentation/modules/restify.js was only in early versions of Node 8. Though this wasn't an issue as it was in the tests, it could have been inside the core code. So running these tests on early intermediate versions does have a little bit of value. I vote for leaving them in for now.

If we later feel that it's too much, we can revisit it and for instance change them to run as a cron job.

What do you think about that?

Qard
Qard approved these changes Dec 6, 2018
View reviewed changes
Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@watson watson merged commit 8395825 into elastic:master Dec 6, 2018
@watson watson deleted the early-versions branch December 6, 2018 15:16
watson added a commit that referenced this pull request Dec 6, 2018
@watson
test(ci): add early supported node versions to test matrix (#705)
0e82351 
Early versions of Node.js 8 had a regression around the handling of
`null` as the port passed to `Server#listen()`. For details see:

nodejs/node#14221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

Assignees

@watson watson

Labels
ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@watson @Qard