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.

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

Local run test failed: benchmark/test-benchmark-napi #34427

Closed
rickyes opened this issue Jul 19, 2020 · 6 comments · Fixed by #34434
Closed

Local run test failed: benchmark/test-benchmark-napi #34427

rickyes opened this issue Jul 19, 2020 · 6 comments · Fixed by #34434

Comments

@rickyes
Copy link
Contributor

rickyes commented Jul 19, 2020

  • Version: master
  • Platform: macOS x64
  • Subsystem: maybe N-API

What steps will reproduce the bug?

make -j4 test following failures will occur:

=== release test-benchmark-napi ===                                           
Path: benchmark/test-benchmark-napi
/Users/ricky/projj/github.com/rickyes/node/benchmark/napi/function_args/index.js: V8 Binding failed to load
misc/function_call.js Binding failed to load
internal/modules/cjs/loader.js:1084
  throw err;
  ^

Error: Cannot find module './build/Release/addon'
Require stack:
- /Users/ricky/projj/github.com/rickyes/node/benchmark/napi/ref/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1081:15)
    at Function.Module._load (internal/modules/cjs/loader.js:927:27)
    at Module.require (internal/modules/cjs/loader.js:1141:19)
    at require (internal/modules/cjs/helpers.js:75:18)
    at Object.<anonymous> (/Users/ricky/projj/github.com/rickyes/node/benchmark/napi/ref/index.js:3:15)
    at Module._compile (internal/modules/cjs/loader.js:1252:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1273:10)
    at Module.load (internal/modules/cjs/loader.js:1101:32)
    at Function.Module._load (internal/modules/cjs/loader.js:966:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/ricky/projj/github.com/rickyes/node/benchmark/napi/ref/index.js'
  ]
}
assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

    at ChildProcess.<anonymous> (/Users/ricky/projj/github.com/rickyes/node/test/common/benchmark.js:30:12)
    at ChildProcess.emit (events.js:314:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/ricky/projj/github.com/rickyes/node/test/benchmark/test-benchmark-napi.js
[02:22|%  88|+ 2779|-   1]: Done                          ^C
make[1]: *** [jstest] Error 1
make: *** [test] Interrupt: 2

How often does it reproduce? Is there a required condition?

Always

@rickyes rickyes changed the title Local run test failed Local run test failed: benchmark/test-benchmark-napi Jul 19, 2020
@Trott
Copy link
Member

Trott commented Jul 20, 2020

make test should not run the benchmark tests. Are you sure this is how you arrived at this error?

The benchmark tests require bench-addons-build so as a workaround, running make bench-addons-build should take care of this. However, bench-addons-build should run automatically if you run make test-benchmark or make test-ci, so I'm interested to know how this came about....

@rickyes
Copy link
Contributor Author

rickyes commented Jul 20, 2020

@Trott I've confirmed the following problem with make test, and by the way, make test-only will get the same error.

@Trott
Copy link
Member

Trott commented Jul 20, 2020

I was able to replicate the problem by running make test-ci-js. bench-addons-build is a prerequisite for test-ci, but it should probably be for test-ci-js. I'll open a PR to fix that at least.

@Trott
Copy link
Member

Trott commented Jul 20, 2020

@Trott I've confirmed the following problem with make test, and by the way, make test-only will get the same error.

That's really puzzling. This is an unmodified master branch? I would think to make benchmark tests run with make test, you'd need to have a modified Makefile or a modified tools.test.py.

@rickyes
Copy link
Contributor Author

rickyes commented Jul 20, 2020

That's really puzzling. This is an unmodified master branch?

Yeah, I haven't changed any of the code to run the make test.

I would think to make benchmark tests run with make test, you'd need to have a modified Makefile or a modified tools.test.py.

As we discussed on #34321, it should be that make test will not run benchmark.

Follow

node/Makefile

Lines 298 to 301 in a51436c

jstest: build-addons build-js-native-api-tests build-node-api-tests ## Runs addon tests and JS tests
$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \
--skip-tests=$(CI_SKIP_TESTS) \
$(CI_JS_SUITES) \
and

node/Makefile

Line 497 in a51436c

CI_JS_SUITES ?= default benchmark
Description, Executing make test does not seem to skip the benchmark.

@Trott
Copy link
Member

Trott commented Jul 20, 2020

Description, Executing make test does not seem to skip the benchmark.

Yes. That's an error. 😞 Will fix....

@Trott Trott closed this as completed in 3caa2e2 Jul 22, 2020
cjihrig pushed a commit that referenced this issue Jul 23, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott added a commit to Trott/io.js that referenced this issue Jul 23, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: nodejs#34427 (comment)

PR-URL: nodejs#34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 28, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 28, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: #34427 (comment)

PR-URL: #34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 28, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 28, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: #34427 (comment)

PR-URL: #34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 29, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this issue Jul 29, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: #34427 (comment)

PR-URL: #34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: #34427 (comment)

PR-URL: #34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Move benchmark CI to native suite since it requires building an addon.

Refs: #34427 (comment)

PR-URL: #34433
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants