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

build: run es-module tests in ci environment #15276

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 8, 2017

I noticed that there was no coverage for @bmeck's new module loading work in CI, I believe the issue might be that es-module was not added to the CI_JS_SUITES variable. I've:

  • updated this variable.
  • updated the test/README listing the new es-module directory.
  • I also updated the table to indicate that abort runs in CI, since I noted this listed in the list of suites listed in CI_JS_SUITES.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows [commit guidelines]
Affected core subsystem(s)

test,build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 8, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@Fishrock123
Copy link
Contributor

Oh no, sorry for overlooking this in my review.

That being said some CI errors in the code in that PR did surface, but perhaps they were elsewhere.

@addaleax
Copy link
Member

addaleax commented Sep 8, 2017

/cc @bmeck for CI errors

@vsemozhetbyt I’d just do another PR for it

@bcoe
Copy link
Contributor Author

bcoe commented Sep 8, 2017

@addaleax @bmeck seeing this one failing test myself on OSX:

=== release test-esm-pkg-over-ext ===                                          
Path: es-module/test-esm-pkg-over-ext
(node:12584) ExperimentalWarning: The ESM module loader is experimental.
Error: module ../fixtures/module-pkg-over-ext/inner not found
    at module.exports (internal/loader/search.js:16:12)
    at resolveRequestUrl (internal/loader/resolveRequestUrl.js:81:13)
    at Loader.getModuleJob (internal/loader/Loader.js:50:27)
    at ModuleWrap.module.link (internal/loader/ModuleJob.js:30:25)
    at linked (internal/loader/ModuleJob.js:28:21)
    at <anonymous>

Any thoughts?


update: see #15280 problem seems to be a missing fixture.

@richardlau
Copy link
Member

There should also be a corresponding change in vcbuild.bat to run the suite on Windows.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 8, 2017

@richardlau done (I think).

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

@bmeck
Copy link
Member

bmeck commented Sep 8, 2017

The resolution of that test might change, due to it having a funny regression. See #14990

@BridgeAR
Copy link
Member

@bmeck this should not be blocking for now though, right?

@targos targos mentioned this pull request Sep 13, 2017
2 tasks
@bmeck
Copy link
Member

bmeck commented Sep 13, 2017

@BridgeAR yup, not blocking; just to note.

@BridgeAR
Copy link
Member

@bcoe would you be so kind and rebase this?

@bcoe bcoe force-pushed the ci-es-module branch 2 times, most recently from 0f433cd to 2c6f451 Compare September 14, 2017 03:55
@bcoe
Copy link
Contributor Author

bcoe commented Sep 14, 2017

@BridgeAR done 👍

@MylesBorins
Copy link
Contributor

@jasnell
Copy link
Member

jasnell commented Sep 14, 2017

CI failed across the board.

@TimothyGu
Copy link
Member

not ok 1664 es-module/test-esm-pkg-over-ext
  ---
  duration_ms: 0.231
  severity: fail
  stack: |-
    (node:39946) ExperimentalWarning: The ESM module loader is experimental.
    Error: module ../fixtures/module-pkg-over-ext/inner not found
        at module.exports (internal/loader/search.js:16:12)
        at resolveRequestUrl (internal/loader/resolveRequestUrl.js:81:13)
        at Loader.getModuleJob (internal/loader/Loader.js:50:27)
        at ModuleWrap.module.link (internal/loader/ModuleJob.js:30:25)
        at linked (internal/loader/ModuleJob.js:28:21)
        at <anonymous>
  ...

@bcoe
Copy link
Contributor Author

bcoe commented Sep 19, 2017

@jasnell not seeing any linting issues when running locally:

Benjamins-MacBook-Pro:node benjamincoe$ make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
	  benchmark doc lib test tools
Running C++ linter...
Total errors found: 0

I'm not seeing any useful output from the Windows CI tests, the only change is that the new es-module module suite has been added to Windows, so I assume this is the culprit.

EDIT: figured out how to navigate Jenkins better:

Error: module ../common not found
    at module.exports (internal/loader/search.js:16:12)
    at resolveRequestUrl (internal/loader/resolveRequestUrl.js:81:13)
    at Loader.getModuleJob (internal/loader/Loader.js:50:27)
    at ModuleWrap.module.link (internal/loader/ModuleJob.js:30:25)
    at linked (internal/loader/ModuleJob.js:28:21)
    at <anonymous>


(node:7092) ExperimentalWarning: The ESM module loader is experimental.
Error: module ./esm-snapshot-mutator not found
    at module.exports (internal/loader/search.js:16:12)
    at resolveRequestUrl (internal/loader/resolveRequestUrl.js:81:13)
    at Loader.getModuleJob (internal/loader/Loader.js:50:27)
    at ModuleWrap.module.link (internal/loader/ModuleJob.js:30:25)
    at linked (internal/loader/ModuleJob.js:28:21)
    at <anonymous>

CC: @bmeck; I can work on getting a Windows testing environment up and running, but might be a while.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 19, 2017

@jasnell @bmeck got the Windows build running tonight and dug a bit more. I think the culprit is in module_wrap.cc, I note that we use a hard-coded / separator for quite a bit of the logic, is there a chance that on Windows one of the operations is resolving to a \ separator?

Not much of a C++ programmer, so might need some direction here @bmeck. tldr; we weren't exercising the es-module code against Windows when it first landed due to the test suite having 35 magic variables that needed to be set whenever a new test folder was added.

@jasnell
Copy link
Member

jasnell commented Sep 19, 2017

Entirely possible. @bmeck would be best bet to look at it but I'll see if I can get to it today (no guarantees)

@bcoe
Copy link
Contributor Author

bcoe commented Sep 19, 2017

I'll get Windows running on my newer laptop today and see if I can dig a bit more too -- was painfully slow to compile on my at home computer.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 23, 2017

@jasnell tests should start passing for windows once we land #15490

Running lint, I'm not quite sure why that build server had issues:

make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
	  benchmark doc lib test tools
Running C++ linter...
Total errors found: 0

I think we should try to land this and #15490 before we land many more es-module features, e.g., #15445, since we're currently not exercising any of the code in CI.

@BridgeAR
Copy link
Member

@bcoe
Copy link
Contributor Author

bcoe commented Sep 25, 2017

@BridgeAR @jasnell this is rebased with the es-module Windows fixes now, so you should be able to kick off a build and have it pass.

@TimothyGu
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Sep 25, 2017

Appears to be working on Windows now, although CI is still running and there appear to be other non-related failures.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 25, 2017

@jasnell @TimothyGu that's looking a lot more promising:

screen shot 2017-09-25 at 3 41 26 pm

Is there a chance that the bindings test is flappy on win2016, I doubt this pull modifies anything related to it:

screen shot 2017-09-25 at 3 41 53 pm

@BridgeAR
Copy link
Member

@bcoe would you be so kind and please rebase?

Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.
@bcoe
Copy link
Contributor Author

bcoe commented Sep 27, 2017

@BridgeAR done.

@BridgeAR
Copy link
Member

refack pushed a commit to refack/node that referenced this pull request Sep 28, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: nodejs#15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Sep 28, 2017

Landed in a1b6cfd

@refack refack closed this Sep 28, 2017
@refack
Copy link
Contributor

refack commented Sep 28, 2017

Quick sanity test on master: https://ci.nodejs.org/job/node-test-commit-linuxone/8875/ ✔️

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: nodejs#15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: #15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: nodejs/node#15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: #15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI.
Update test/README adding es-module section.

PR-URL: #15276
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.