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.

Sign up for GitHub

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

Jump to bottom

node-test-node-addon-api-new job hanging? #2131

Closed
BethGriggs opened this issue Jan 14, 2020 · 14 comments
Closed

node-test-node-addon-api-new job hanging? #2131

BethGriggs opened this issue Jan 14, 2020 · 14 comments
Labels
ci-change PSA of configuration changes

Comments

@BethGriggs
Copy link
Member

It seems like this job is regularly hanging - previous runs have taken between 2-10minutes. The previous run took 7 hours+ and the current run is at 4+ hours.

https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=fedora-latest-x64/1617/
@BethGriggs
Copy link
Member Author

The same test hang across multiple platforms - possibly caused by nodejs/node@689ab46?

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   message: 'Invalid argument',
+   name: 'Error'
-   message: 'Maximum BigInt size exceeded',
-   name: 'RangeError'
  }
    at test (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/centos7-ppcle/node-addon-api/test/bigint.js:48:10)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/centos7-ppcle/node-addon-api/test/bigint.js:6:1)
    at Module._compile (internal/modules/cjs/loader.js:1215:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1235:10)
    at Module.load (internal/modules/cjs/loader.js:1064:32)
    at Function.Module._load (internal/modules/cjs/loader.js:959:14)
    at Module.require (internal/modules/cjs/loader.js:1104:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/centos7-ppcle/node-addon-api/test/index.js:96:5
    at Array.forEach (<anonymous>) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: Error: Invalid argument
      at getActual (assert.js:688:5)
      at Function.throws (assert.js:828:24)
      at test (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/centos7-ppcle/node-addon-api/test/bigint.js:48:10)
      at Object.<anonymous> (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/centos7-ppcle/node-addon-api/test/bigint.js:6:1)
      at Module._compile (internal/modules/cjs/loader.js:1215:30)
      at Object.Module._extensions..js (internal/modules/cjs/loader.js:1235:10)
      at Module.load (internal/modules/cjs/loader.js:1064:32)
      at Function.Module._load (internal/modules/cjs/loader.js:959:14)
      at Module.require (internal/modules/cjs/loader.js:1104:19)
      at require (internal/modules/cjs/helpers.js:72:18),
  expected: [Object],
  operator: 'throws'
}

ping @nodejs/addon-api

@BethGriggs
Copy link
Member Author

Aborted https://ci.nodejs.org/job/node-test-node-addon-api-new/1616/ and https://ci.nodejs.org/job/node-test-node-addon-api-new/1617/ as they were both hanging.

@richardlau
Copy link
Member

richardlau commented Jan 14, 2020 •
edited
Loading

ping @nodejs/addon-api

Based on the team members listed on https://github.com/nodejs/node-addon-api maybe also ping @nodejs/n-api?

@BethGriggs
Copy link
Member Author

Does it make sense to turn the timer off on that job until the hanging issue is resolved? I think it is configured to run daily.

@richardlau
Copy link
Member

@nodejs/n-api-admins Are there any objections to enabling timeouts on the job to prevent hanging tests from tying up CI infrastructure?
image

@mhdawson
Copy link
Member

I'll try to look today to see if its the BigInt change and if we need to update the tests.

@mhdawson
Copy link
Member

Cancelled the current job

@mhdawson
Copy link
Member

Started by removing 8.x from the tests as is has gone EOL and therefore we've landed some code that no longer works there.

@mhdawson
Copy link
Member

Next will look at the test so that it accomodates the change in master.

@richardlau
Copy link
Member

@mhdawson @nodejs/n-api-admins Any comments on adding a timeout to the node-test-node-addon-api-new build (#2131 (comment))?

I've also noticed that node-test-node-addon-api-new runs once a day on the sixth hour:
image

but it's also run once per day by node-test-node-addon-api-LTS versions on the third hour:
image

(e.g. for today https://ci.nodejs.org/job/node-test-node-addon-api-new/1625/ and https://ci.nodejs.org/job/node-test-node-addon-api-new/1623/ (via https://ci.nodejs.org/job/node-test-node-addon-api-LTS%20versions/617/))

Does master need to be tested twice a day?

mhdawson added a commit to mhdawson/node-addon-api that referenced this issue Jan 15, 2020
@mhdawson
test: update BigInt test for recent change in core
c38baac 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.
@mhdawson mhdawson mentioned this issue Jan 15, 2020
Closed
@mhdawson
Copy link
Member

PR to fix test: nodejs/node-addon-api#649

Believe it was also likely the cause of the hang.

mhdawson added a commit to nodejs/node-addon-api that referenced this issue Jan 15, 2020
@mhdawson
test: update BigInt test for recent change in core
2fde5c3 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: #649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
@mhdawson
Copy link
Member

Landed PR to fix test.

Updated the new job to not run on its own so we should now only test master once a day

Launched job to test across versions to confirm we are now good
https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-LTS%20versions/618/

@mhdawson
Copy link
Member

CI run was good across all versions, closing.

@richardlau I'm ok if you enable the option to abort if stuck.

@richardlau
Copy link
Member

Done: https://ci.nodejs.org/job/node-test-node-addon-api-new/jobConfigHistory/showDiffFiles?timestamp1=2020-01-15_16-24-35&timestamp2=2020-01-15_18-16-18

Added a no activity 3600 second (1 hour) time out.

@richardlau richardlau added the ci-change PSA of configuration changes label Jan 15, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue May 11, 2020
@mhdawson
test: update BigInt test for recent change in core
e58b770 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Jun 12, 2020
@mhdawson
test: update BigInt test for recent change in core
81e2eac 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
@kevindavies8
test: update BigInt test for recent change in core
f8ceb67 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs/node-addon-api#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
@Marlyfleitas
test: update BigInt test for recent change in core
9676172 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs/node-addon-api#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
@wroy7860
test: update BigInt test for recent change in core
d32bde1 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs/node-addon-api#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
test: update BigInt test for recent change in core
20efb3f 
nodejs/node@689ab46
changed the expected error for one of the BigInt test cases. This is
ok because BigInt is still in experimental. However, the
result was a failed/hanging test. See
nodejs/build#2131

This changes the test to accept either the new or old behaviour.
We need that so that older versions of Node.js will also pass the
test until the the node core commit above is backported.

PR-URL: nodejs/node-addon-api#649
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ci-change PSA of configuration changes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@richardlau @BethGriggs @mhdawson