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

test: add stdout check to test-esm-cjs-main #25073

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Trott
Copy link
Member

Trott commented Dec 16, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@Trott

This comment has been minimized.

@richardlau
Copy link
Member

richardlau left a comment

LGTM if we rename the parameter.

Show resolved Hide resolved test/es-module/test-esm-cjs-main.js Outdated
@Trott

This comment has been minimized.

@Trott Trott added the author ready label Dec 16, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

Note to whoever lands this (which will probably be me): The commit message needs to be edited as it's checking signal rather than stdout.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

Only red in the CI seems to be because the cleanup detected that @gireeshpunathil was testing something on CI:

21:45:21 ps awwx | grep Release/node | grep -v grep | cat
21:45:21  32506076  pts/2 A     0:00 /home/iojs/gireesh/exitrace/node/out/Release/node -e console.log('foobar'); 
21:45:21 gmake[1]: *** [test-ci] Error 1

If we see this happening a lot, we'll mark the node offline in Jenkins while they're doing their work (which is something we should do anyway, but we'd need some pretty responsive/on-duty Build folks to oversee it because right now we have them on one AIX node and @gabrielschulhof working on the other, so we'd have no AIX nodes left online potentially).

TL;DR Red CI here is unrelated and we'll Resume Build anyway.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

@lpinca

lpinca approved these changes Dec 16, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 18, 2018

Landed in dbdfc5d

@Trott Trott closed this Dec 18, 2018

Trott added a commit to Trott/io.js that referenced this pull request Dec 18, 2018

test: add signal check to test-esm-cjs-main
PR-URL: nodejs#25073
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 25, 2018

test: add signal check to test-esm-cjs-main
PR-URL: #25073
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

test: add signal check to test-esm-cjs-main
PR-URL: nodejs#25073
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment