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

node-test-commit matrix document #1865

Closed
wants to merge 3 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jul 18, 2019

I was writing up some basic notes on our standard CI matrix and figured it might make a good doc for our repo even though it's not likely to stay updated (hence the caveats at the top).

It's highlighted a couple of things already:

  1. node-test-commit-custom-suites-freestyle is really far too complex for what it contributes to node-test-commit and could be easily refactored out into the containered jobs (from what I can tell) Removing node-test-commit-custom-suites-freestyle from node-test-commit #1864
  2. The debian9-docker-armv7 label was missing from node-test-commit-linux-arm, it should have been there since Node 12 but I guess this was my oversight. I've added it and am testing it out https://ci.nodejs.org/job/node-test-commit-arm/25379/nodes=debian9-docker-armv7/console
  3. The containered jobs config could be refactored into a script in this repo. As the list of them as grown, the copypasta has increased, there's a standard set of things that need to be executed for each of them and they could be put into a single script, or at least a collection of scripts which would make it easier to diagnose problems like the recent one that was highlighted where debug builds aren't running the full compliment of tests.

@joaocgreis the Windows matrix doesn't seem to be using VersionSelectorScript.groovy, I've tried my best to reproduce what I think the rules are but a sanity check from you would be appreciated.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@rvagg
Copy link
Member Author

rvagg commented Jul 22, 2019

Need some Windows sanity check before I can merge this I think.

@joaocgreis
Copy link
Member

@rvagg pushed a fixup with some adjustments, feel free to adjust as you see fit.

Windows doesn't use the VersionSelectorScript because of two issues (at least):

  • Would have to separate what can build and what should be used to build the official releases. For example, for Node 8, we want to use both VS2015 and VS2017 to build in CI and we have those two versions as well in the release Jenkins (where VS2015 should be used).
  • We support running (including building addons) in more versions than we support compiling core, and we want to test those in CI.

Separating the test jobs in CI as we have now for Node >= 11 is not a bad way of organizing it. So, even though VersionSelectorScript would probably be a better way I don't think adapting it is urgent at the moment.

@rvagg
Copy link
Member Author

rvagg commented Jul 26, 2019

thanks @joaocgreis, just having the windows matrix outlined like this is helpful

rvagg added a commit that referenced this pull request Jul 26, 2019
@rvagg
Copy link
Member Author

rvagg commented Jul 26, 2019

merged

@rvagg rvagg closed this Jul 26, 2019
@rvagg rvagg deleted the rvagg/node-test-commit-matrix.md branch July 26, 2019 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants