test: fixes for multiplatform ChakraCore CI #233

joaocgreis · 2017-05-02T18:17:47Z

This PR should make the ChakraCore CI jobs pass on the current xplat head. This includes 3 commits:

7f8b1ce: Fixes some differences to node/master that probably resulted from merge operations. Most are whitespace differences.
b803dec: Mark all tests that fail in CI for ChakraCore but not for V8 as flaky in the status files. This makes the build yellow, not green, so that these tests are not forgotten. Also uses common.skip() coherently throughout the tests that are skipped for ChakraCore (edited, was in a separate commit previously).
db75280: skips test-child-process-pass-fd, it makes Jenkins fail the test run but the test itself passes locally. I will investigate further and revert this commit as soon as possible.

Tests that have issues with ChakraCore should fall in one of two categories:

Tests that are V8 specific: Should be skipped in the beginning of the test code with common.skip() (this allows the build to be green).
Tests that are not yet working with ChakraCore, but should in the future: should be marked as flaky in the status files (this makes the build yellow, so that these tests are not forgotten).

cc @nodejs/node-chakracore

Checklist

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

Affected core subsystem(s)

test

joaocgreis · 2017-05-02T18:19:41Z

CI: https://ci.nodejs.org/view/All/job/chakracore-test-pull-request/13/

agarwal-sandeep · 2017-05-02T18:26:00Z

LGTM

kfarnung

LGTM

kunalspathak

I would first merge 47f8f74 in node-chakracore without manually deleting these files as a separate commit to avoid merge conflicts.

joaocgreis · 2017-05-02T18:48:52Z

@kunalspathak what do you mean? 47f8f74 was already merged in the last merge PR (#211), so it's already an ancestor of this commit. Those files should have been deleted as part of the merge.

kunalspathak · 2017-05-02T18:50:45Z

I see, in that case are you saying that those files are still present in node-chakracore because of bad merge? Why is the need to delete them again in your commit?

joaocgreis · 2017-05-02T19:05:32Z

@kunalspathak yes, they were not deleted during the merge. I don't see a reason to keep them here, do they test functionality that is still relevant in node-chackracore but not v8? I compared the test/ from xplat with the node/master parent of the last merge commit, and removed every change that I did not see a reason for (that I believe should have happened in the merges). It is useful to have the diff as clean as possible, to identify what changed in node-chakracore.

kunalspathak · 2017-05-02T20:20:08Z

It is useful to have the diff as clean as possible, to identify what changed in node-chakracore.

@joaocgreis , I agree with you. I was just trying to understand if this was the result of bad merge. Thanks for confirming and yes, please delete those files since they are already deleted in nodejs/master.

Your changes LGTM.

joaocgreis · 2017-05-08T18:43:41Z

Rebased and updated: tests should now fall strictly in the two categories mentioned in the OP. @kunalspathak thanks for your help with this.

@agarwal-sandeep @kfarnung this is now different than the version you previously stamped, let me know if you want time to review this again. Thanks!

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/23/

joaocgreis · 2017-05-09T12:57:47Z

I added a commit to fix a linter error. Can someone take a look to make sure it's good to land? Thanks!

kunalspathak · 2017-05-09T16:55:34Z

test/sequential/sequential.status

@@ -22,4 +22,5 @@ test-benchmark-child-process  : PASS,FLAKY
 [$jsEngine==chakracore]

 [$jsEngine==chakracore && $system==linux]
-test-child-process-pass-fd : PASS,FLAKY
+# This test crashes Jenkins in Linux CI
+test-child-process-pass-fd : SKIP


if this test crashes Jenkins in Linux CI, why is it under $jsEngine==chakracore condition? Or is it only crashing CI setup for chakracore?

Only for chakracore. I'm investigating this, I'll report when I find the cause.

kunalspathak · 2017-05-09T16:57:00Z

lib/trace_mgr.js

@@ -353,7 +353,7 @@ function checkGlobalShouldEmit(emitKind, optInfo) {
    } else {
      return (emitOptions.emitOnExit === 'error') && (optInfo !== 0);
    }
-  } else if (emitKind == 'emitOnException' || emitKind == 'emitOnSigInt') {
+  } else if (emitKind === 'emitOnException' || emitKind === 'emitOnSigInt') {


Kyle already fixed it in 4751de5#diff-d3e8e0bdb7af6cb1b0059f4d2d23e4b1

Thanks! I'll remove the commit.

Deleted files were deleted in node/master in 47f8f74. Tests for GH-7849 and GH-5110 were moved from test-buffer-alloc to test-buffer-tojson in f3dc6a3. PR-URL: nodejs#233 Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Kunal Pathak <Kunal.Pathak@microsoft.com>

Mark as flaky tests that fail with ChakraCore on CI. Tests that are V8-specific and therefore will never run should be skipped in the beginning of the test code, not in the status files. PR-URL: nodejs#233 Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Kunal Pathak <Kunal.Pathak@microsoft.com>

This test passes when run locally, but causes Jenkins CI to fail. PR-URL: nodejs#233 Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Kunal Pathak <Kunal.Pathak@microsoft.com>

joaocgreis · 2017-05-10T18:51:30Z

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/27/

Landed in 4751de5...7e6de45

kfarnung approved these changes May 2, 2017

View reviewed changes

kunalspathak reviewed May 2, 2017

View reviewed changes

kfarnung mentioned this pull request May 2, 2017

chakrashim: Updated ChakraCore to release/2.0 #232

Merged

2 tasks

kunalspathak mentioned this pull request May 4, 2017

Merge nodejs/master to xplat #236

Merged

2 tasks

joaocgreis force-pushed the joaocgreis-H4M-chakracore-test-status branch from 1bde7e1 to db75280 Compare May 8, 2017 18:09

kunalspathak reviewed May 9, 2017

View reviewed changes

kfarnung approved these changes May 9, 2017

View reviewed changes

joaocgreis added 2 commits May 10, 2017 14:31

joaocgreis force-pushed the joaocgreis-H4M-chakracore-test-status branch from e4f6460 to 844a466 Compare May 10, 2017 13:54

joaocgreis force-pushed the joaocgreis-H4M-chakracore-test-status branch from 844a466 to 7e6de45 Compare May 10, 2017 16:08

joaocgreis merged commit 7e6de45 into nodejs:xplat May 10, 2017

joaocgreis mentioned this pull request Sep 29, 2017

V8.6.0 proposal #405

Merged

4 tasks

joaocgreis mentioned this pull request Nov 15, 2017

Skip flaky tests by default? #426

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fixes for multiplatform ChakraCore CI #233

test: fixes for multiplatform ChakraCore CI #233

joaocgreis commented May 2, 2017 •

edited

Loading

joaocgreis commented May 2, 2017

agarwal-sandeep commented May 2, 2017

kfarnung left a comment

kunalspathak left a comment

joaocgreis commented May 2, 2017

kunalspathak commented May 2, 2017

joaocgreis commented May 2, 2017

kunalspathak commented May 2, 2017

joaocgreis commented May 8, 2017

joaocgreis commented May 9, 2017

kunalspathak May 9, 2017

joaocgreis May 10, 2017

kunalspathak May 9, 2017

joaocgreis May 10, 2017

joaocgreis commented May 10, 2017

test: fixes for multiplatform ChakraCore CI #233

test: fixes for multiplatform ChakraCore CI #233

Conversation

joaocgreis commented May 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

joaocgreis commented May 2, 2017

agarwal-sandeep commented May 2, 2017

kfarnung left a comment

Choose a reason for hiding this comment

kunalspathak left a comment

Choose a reason for hiding this comment

joaocgreis commented May 2, 2017

kunalspathak commented May 2, 2017

joaocgreis commented May 2, 2017

kunalspathak commented May 2, 2017

joaocgreis commented May 8, 2017

joaocgreis commented May 9, 2017

kunalspathak May 9, 2017

Choose a reason for hiding this comment

joaocgreis May 10, 2017

Choose a reason for hiding this comment

kunalspathak May 9, 2017

Choose a reason for hiding this comment

joaocgreis May 10, 2017

Choose a reason for hiding this comment

joaocgreis commented May 10, 2017

joaocgreis commented May 2, 2017 •

edited

Loading