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

test: run test-abort-aliased-buffer-overflow #32626

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

#31740 added an addon-style test to
test/abort that was never run because test/abort/testcfg.py uses the
AbortTestConfiguration which inherits from SimpleTestConfiguration which
only finds tests in the root of test/abort.

Make AbortTestConfiguration inherit from AddonTestConfiguration and
change AddonTestConfiguration to find the tests in the root of the test
bucket in addition to the subfolders.

Fixup test-abort-aliased-buffer-overflow so that it works as intended.

Refs: #31740

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

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 3, 2020
@richardlau
Copy link
Member Author

Looks like the test is failing on Windows.

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

Updated to fix the Windows build. The workers CI is also failing, but I'll need to think for a bit (when it's not 3am in the morning) how to fix that because I can't just add make build-abort-tests to the job config because that target doesn't exist in any of the release lines so would break the job if run on those.

nodejs#31740 added an addon-style test to
`test/abort` that was never run because `test/abort/testcfg.py` uses the
AbortTestConfiguration which inherits from SimpleTestConfiguration which
only finds tests in the root of `test/abort`.

Make AbortTestConfiguration inherit from AddonTestConfiguration and
change AddonTestConfiguration to find the tests in the root of the test
bucket in addition to the subfolders.

Fixup `test-abort-aliased-buffer-overflow` so that it works as intended.

Signed-off-by: Richard Lau <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

Fixed most of the broken CI by removing the abort tests from default since they now require compilation

node/tools/test.py

Lines 1487 to 1489 in b9da063

# these suites represent special cases that should not be run as part of the
# default JavaScript test-run, e.g., internet/ requires a network connection,
# addons/ requires compilation.
.

Compilation of the test is failing on Windows, e.g. https://ci.nodejs.org/job/node-test-binary-windows-native-suites/2398/nodes=win10-vs2019-COMPILED_BY-vs2019/console

03:51:56   stdout: 'Building the projects in this solution one at a time. To enable parallel build, please add the "-m" switch.\r\n' +
03:51:56     '  binding.cc\r\n' +
03:51:56     '  win_delay_load_hook.cc\r\n' +
03:51:56     '     Creating library C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\abort\\test_abort-aliased-buffer-overflow\\build\\Release\\binding.lib and object C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\abort\\test_abort-aliased-buffer-overflow\\build\\Release\\binding.exp\r\n' +
03:51:56     'binding.obj : error LNK2019: unresolved external symbol "void __cdecl node::Assert(struct node::AssertionInfo const &)" (?Assert@node@@YAXAEBUAssertionInfo@1@@Z) referenced in function "void __cdecl AllocateAndResizeBuffer(class v8::FunctionCallbackInfo<class v8::Value> const &)" (?AllocateAndResizeBuffer@@YAXAEBV?$FunctionCallbackInfo@VValue@v8@@@v8@@@Z) [C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\abort\\test_abort-aliased-buffer-overflow\\build\\binding.vcxproj]\r\n' +
03:51:56     'C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\abort\\test_abort-aliased-buffer-overflow\\build\\Release\\binding.node : fatal error LNK1120: 1 unresolved externals [C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\abort\\test_abort-aliased-buffer-overflow\\build\\binding.vcxproj]\r\n',

Is the Aliased*Buffer API external (in which case this looks like a bug)? If it's internal then this really shouldn't be tested as an addon (particularly with all the build file and test configuration changes it dragged in with it) and if we really want a test for it maybe as a cctest? (Although I don't know how cctest would handle testing that something aborts.)

@richardlau
Copy link
Member Author

I'm going to close this in favour of reverting the test (just the test+Makefile changes): #33196

It doesn't seem that AliasedBuffers should be tested as an addon, and mixing addons
and non-addons tests in the same test bucket causes complications in our CI (as on
some platforms we separate native (i.e. requires compilation) tests from pure
JavaScript tests).

@richardlau richardlau closed this May 1, 2020
@richardlau richardlau deleted the aborttestcfg branch May 1, 2020 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants