test: fix test-esm-addons #16174

Trott · 2017-10-13T05:06:17Z

Move test-esm-addons to test/addons/hello-world-esm.

Test should now work properly on CI machines where addons are not
always built at the expected relative path from the es-modules tests.

Test should now work in Debug builds.

Fixes: #16155

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Trott · 2017-10-13T05:07:27Z

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

Trott · 2017-10-13T05:08:47Z

Assuming no CI surprises, would love to see this fast-tracked to get CI back to yellow/green.

@guybedford @refack @bmeck @danbev @nodejs/testing

addaleax

LGTM anyway

addaleax · 2017-10-13T05:08:49Z

test/addons/hello-world-esm/test.mjs

 /* eslint-disable required-modules */

 import assert from 'assert';
-import binding from '../addons/hello-world/build/Release/binding.node';
+import binding from './build/Release/binding.node';


This still uses build/Release… did you verify that this fixes #16155?

Whoops, I'll fix that. (No, I didn't verify. In my defense, I said "should now work in Debug builds" rather than "definitely does now work in Debug builds". :-D )

Fixed! Should now work in Debug builds for really realz, although I haven't verified.

Move test-esm-addons to test/addons/hello-world-esm. Test should now work properly on CI machines where `addons` are not always built at the expected relative path from the es-modules tests. Test should now work in Debug builds. Fixes: nodejs#16155

Trott · 2017-10-13T05:13:49Z

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

Trott · 2017-10-13T05:16:35Z

I haven't verified that this fix makes the test pass on Debug builds, so if anyone verifies that, it would be awesome if you could leave a note here.

addaleax

Yup, LGTM & I’ll try this out in a debug build later

addaleax · 2017-10-13T05:21:57Z

test/addons/hello-world-esm/test.js

+const buildDir = join(__dirname, 'build');
+
+copyFileSync(join(buildDir, common.buildType, 'binding.node'),
+             join(buildDir, 'binding.node'));


We should maybe just do this (or, on POSIX, do symlinks) for all addons.

Trott · 2017-10-13T05:36:17Z

CI is showing green, but no tests were actually run. Something is not working on CI. Will have to run again after it's fixed.

danbev · 2017-10-13T05:37:40Z

I haven't verified that this fix makes the test pass on Debug builds, so if anyone verifies that, it would be awesome if you could leave a note here.

Debug build worked for me 👍

Trott · 2017-10-13T05:42:05Z

CI seems to be working now.

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

Trott · 2017-10-13T06:27:32Z

Linux had an unrelated failure so I re-ran and its green: https://ci.nodejs.org/job/node-test-commit-linux/13135/

Everything else is green the first time including those Raspberry Pi devices that were failing on this test before. I'm going to sleep, but if someone feels that this is worth fast-tracking, you certainly have my endorsement to land it. :-D

guybedford · 2017-10-13T10:25:12Z

Thanks so much for this fix, and apologies - it completely slipped my mind to find a more portable approach here. Just caught this now due to time zones, but looks great to me!

refack · 2017-10-13T12:34:37Z

K.I.S.S. FTW. I got stuck in my own head trying to find an elegant solution for

node/test/testpy/__init__.py

Lines 149 to 166 in 668f4cf

    
           def Ls(self, path): 
        
             def SelectTest(name): 
        
               return name.endswith('.js') 
        
             result = [] 
        
             for subpath in os.listdir(path): 
        
               if os.path.isdir(join(path, subpath)): 
        
                 for f in os.listdir(join(path, subpath)): 
        
                   if SelectTest(f): 
        
                     result.append([subpath, f[:-3]]) 
        
             return result 
        
           def ListTests(self, current_path, path, arch, mode): 
        
             all_tests = [current_path + t for t in self.Ls(join(self.root))] 
        
             result = [] 
        
             for test in all_tests: 
        
               if self.Contains(path, test): 
        
                 file_path = join(self.root, reduce(join, test[1:], "") + ".js")

refack · 2017-10-13T17:03:08Z

@Trott I'm thinking maybe duplicating a whole addon just for this test is too expensive... Any thoughts?

Trott · 2017-10-13T17:28:27Z

@Trott I'm thinking maybe duplicating a whole addon just for this test is too expensive... Any thoughts?

I prefer it as a separate unit with no dependencies on other tests, but I wouldn't oppose a subsequent refactoring that reduced that duplication if that's bothering you or anyone else. I'm thinking this should land as-is to get the CI to not-red as soon as possible.

Move test-esm-addons to test/addons/hello-world-esm. Test should now work properly on CI machines where `addons` are not always built at the expected relative path from the es-modules tests. Test should now work in Debug builds. PR-URL: nodejs#16174 Fixes: nodejs#16155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Trott · 2017-10-13T17:51:23Z

Landed in 981595c. I thought I saw someone else give a thumbs-up to fast-tracking, but now I don't see it. Apologies if this went too fast...

Move test-esm-addons to test/addons/hello-world-esm. Test should now work properly on CI machines where `addons` are not always built at the expected relative path from the es-modules tests. Test should now work in Debug builds. PR-URL: nodejs/node#16174 Fixes: nodejs/node#16155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Move test-esm-addons to test/addons/hello-world-esm. Test should now work properly on CI machines where `addons` are not always built at the expected relative path from the es-modules tests. Test should now work in Debug builds. PR-URL: #16174 Fixes: #16155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 13, 2017

nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Oct 13, 2017

This was referenced Oct 13, 2017

test-esm-addon.mjs fails when building in debug mode #16155

Closed

module: resolve and instantiate loader pipeline hooks #15445

Closed

addaleax approved these changes Oct 13, 2017

View reviewed changes

test: fix test-esm-addons

e32a9fe

Move test-esm-addons to test/addons/hello-world-esm. Test should now work properly on CI machines where `addons` are not always built at the expected relative path from the es-modules tests. Test should now work in Debug builds. Fixes: nodejs#16155

Trott force-pushed the fix-the-ding-dang-ci branch from 3ae9e0f to e32a9fe Compare October 13, 2017 05:12

addaleax approved these changes Oct 13, 2017

View reviewed changes

danbev approved these changes Oct 13, 2017

View reviewed changes

Trott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Oct 13, 2017

refack approved these changes Oct 13, 2017

View reviewed changes

Trott closed this Oct 13, 2017

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Trott deleted the fix-the-ding-dang-ci branch January 13, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test-esm-addons #16174

test: fix test-esm-addons #16174

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

addaleax left a comment

addaleax Oct 13, 2017

Trott Oct 13, 2017

Trott Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

addaleax left a comment

addaleax Oct 13, 2017

Trott commented Oct 13, 2017

danbev commented Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

guybedford commented Oct 13, 2017

refack commented Oct 13, 2017

refack commented Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

test: fix test-esm-addons #16174

test: fix test-esm-addons #16174

Conversation

Trott commented Oct 13, 2017

Checklist

Affected core subsystem(s)

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax Oct 13, 2017

Choose a reason for hiding this comment

Trott Oct 13, 2017

Choose a reason for hiding this comment

Trott Oct 13, 2017

Choose a reason for hiding this comment

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax Oct 13, 2017

Choose a reason for hiding this comment

Trott commented Oct 13, 2017

danbev commented Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017

guybedford commented Oct 13, 2017

refack commented Oct 13, 2017

refack commented Oct 13, 2017

Trott commented Oct 13, 2017

Trott commented Oct 13, 2017