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

bcoe · 2017-09-08T15:44:19Z

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

addaleax

CI: https://ci.nodejs.org/job/node-test-commit/12257/

Fishrock123 · 2017-09-08T15:54:03Z

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 · 2017-09-08T16:00:34Z

/cc @bmeck for CI errors

@vsemozhetbyt I’d just do another PR for it

bcoe · 2017-09-08T16:13:40Z

@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 · 2017-09-08T17:41:53Z

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

bcoe · 2017-09-08T17:55:01Z

@richardlau done (I think).

mhdawson

LGTM

bmeck · 2017-09-08T19:56:41Z

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

BridgeAR · 2017-09-13T19:58:24Z

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

bmeck · 2017-09-13T22:04:02Z

@BridgeAR yup, not blocking; just to note.

BridgeAR · 2017-09-13T22:08:11Z

@bcoe would you be so kind and rebase this?

bcoe · 2017-09-14T03:56:37Z

@BridgeAR done 👍

MylesBorins · 2017-09-14T11:52:31Z

One more time https://ci.nodejs.org/job/node-test-pull-request/10074/

jasnell · 2017-09-14T18:22:01Z

CI failed across the board.

TimothyGu · 2017-09-14T18:24:52Z

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 · 2017-09-19T00:31:51Z

@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 · 2017-09-19T07:40:33Z

@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 · 2017-09-19T14:24:52Z

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 · 2017-09-19T16:31:06Z

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 · 2017-09-23T00:50:12Z

@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 · 2017-09-24T20:03:06Z

CI https://ci.nodejs.org/job/node-test-pull-request/10242/

bcoe · 2017-09-25T18:19:11Z

@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 · 2017-09-25T21:49:26Z

@bcoe Sure!

CI: https://ci.nodejs.org/job/node-test-pull-request/10260/

jasnell · 2017-09-25T22:37:44Z

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

bcoe · 2017-09-25T22:43:18Z

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

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

BridgeAR · 2017-09-27T21:13:59Z

@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 · 2017-09-27T22:16:05Z

@BridgeAR done.

BridgeAR · 2017-09-27T22:46:18Z

CI https://ci.nodejs.org/job/node-test-pull-request/10295/

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 · 2017-09-28T01:48:12Z

Landed in a1b6cfd

refack · 2017-09-28T01:49:21Z

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

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]>

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]>

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]>

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]>

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

addaleax approved these changes Sep 8, 2017

View reviewed changes

Fishrock123 approved these changes Sep 8, 2017

View reviewed changes

TimothyGu approved these changes Sep 8, 2017

View reviewed changes

bcoe force-pushed the ci-es-module branch from 2be8791 to 19a5b97 Compare September 8, 2017 16:49

bcoe mentioned this pull request Sep 8, 2017

test: missing fixture test-esm-pkg-over-ext.mjs #15280

Closed

2 tasks

bcoe force-pushed the ci-es-module branch from 19a5b97 to 3460e66 Compare September 8, 2017 17:54

richardlau approved these changes Sep 8, 2017

View reviewed changes

mhdawson approved these changes Sep 8, 2017

View reviewed changes

lpinca approved these changes Sep 9, 2017

View reviewed changes

jasnell approved these changes Sep 12, 2017

View reviewed changes

BridgeAR approved these changes Sep 13, 2017

View reviewed changes

targos mentioned this pull request Sep 13, 2017

Possible fix for #15374 #15389

Closed

2 tasks

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

TimothyGu approved these changes Sep 14, 2017

View reviewed changes

danbev approved these changes Sep 14, 2017

View reviewed changes

bcoe force-pushed the ci-es-module branch from 4cd3259 to 9b0ad13 Compare September 25, 2017 17:25

hiroppy approved these changes Sep 25, 2017

View reviewed changes

build: run es-module tests in ci environment

00d25c3

Add es-module to CI_JS_SUITES/js_test_suites, so that tests run in CI. Update test/README adding es-module section.

bcoe force-pushed the ci-es-module branch from 9b0ad13 to 00d25c3 Compare September 27, 2017 22:12

refack approved these changes Sep 28, 2017

View reviewed changes

refack closed this Sep 28, 2017

guybedford mentioned this pull request Sep 28, 2017

module: resolve and instantiate loader pipeline hooks #15445

Closed

4 tasks

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bcoe commented Sep 8, 2017 •

edited

Loading

addaleax left a comment

Fishrock123 commented Sep 8, 2017

addaleax commented Sep 8, 2017

bcoe commented Sep 8, 2017 •

edited

Loading

richardlau commented Sep 8, 2017

bcoe commented Sep 8, 2017

mhdawson left a comment

bmeck commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

bmeck commented Sep 13, 2017

BridgeAR commented Sep 13, 2017

bcoe commented Sep 14, 2017

MylesBorins commented Sep 14, 2017

jasnell commented Sep 14, 2017

TimothyGu commented Sep 14, 2017

bcoe commented Sep 19, 2017 •

edited

Loading

bcoe commented Sep 19, 2017 •

edited

Loading

jasnell commented Sep 19, 2017

bcoe commented Sep 19, 2017

bcoe commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

bcoe commented Sep 25, 2017

TimothyGu commented Sep 25, 2017

jasnell commented Sep 25, 2017

bcoe commented Sep 25, 2017

BridgeAR commented Sep 27, 2017

bcoe commented Sep 27, 2017

BridgeAR commented Sep 27, 2017

refack commented Sep 28, 2017

refack commented Sep 28, 2017 •

edited

Loading

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

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

Conversation

bcoe commented Sep 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Fishrock123 commented Sep 8, 2017

addaleax commented Sep 8, 2017

bcoe commented Sep 8, 2017 • edited Loading

richardlau commented Sep 8, 2017

bcoe commented Sep 8, 2017

mhdawson left a comment

Choose a reason for hiding this comment

bmeck commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

bmeck commented Sep 13, 2017

BridgeAR commented Sep 13, 2017

bcoe commented Sep 14, 2017

MylesBorins commented Sep 14, 2017

jasnell commented Sep 14, 2017

TimothyGu commented Sep 14, 2017

bcoe commented Sep 19, 2017 • edited Loading

bcoe commented Sep 19, 2017 • edited Loading

jasnell commented Sep 19, 2017

bcoe commented Sep 19, 2017

bcoe commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

bcoe commented Sep 25, 2017

TimothyGu commented Sep 25, 2017

jasnell commented Sep 25, 2017

bcoe commented Sep 25, 2017

BridgeAR commented Sep 27, 2017

bcoe commented Sep 27, 2017

BridgeAR commented Sep 27, 2017

refack commented Sep 28, 2017

refack commented Sep 28, 2017 • edited Loading

bcoe commented Sep 8, 2017 •

edited

Loading

bcoe commented Sep 8, 2017 •

edited

Loading

bcoe commented Sep 19, 2017 •

edited

Loading

bcoe commented Sep 19, 2017 •

edited

Loading

refack commented Sep 28, 2017 •

edited

Loading