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: define and test validity of core-dumps #14110

Closed
refack opened this issue Jul 6, 2017 · 7 comments
Closed

test: define and test validity of core-dumps #14110

refack opened this issue Jul 6, 2017 · 7 comments
Labels
post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Jul 6, 2017

  • Version: *
  • Platform: *
  • Subsystem: test,process

The ad-hoc situation is that node has incorporated V8's --abort-on-uncaught-exception as a core feature (Ref: #14013).
There seems to be a consensus forming to make this an explicit design decision and document it (Ref: #13931).

Currently the test suite only tests for process exit state (i.e. code and signal) but does not do any testing on the validity of generated code-dump.

I suggest we define the minimal requirements needed from the core-dump to be valid and find a way to automatically assert those.

/cc @nodejs/post-mortem @nodejs/testing @nodejs/release @nodejs/build

@refack refack added post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. labels Jul 6, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jul 6, 2017

FWIW, the llnode test suite generates core files using --abort-on-uncaught-exception, loads them into lldb, and executes commands. Since that project lives under the nodejs org, IMO that should be enough validation.

@richardlau
Copy link
Member

AFAIK llnode/lldb doesn't yet cover all of our supported platforms.

@richardlau
Copy link
Member

For the platforms it does cover, see nodejs/build#777 about adding CI for llnode.

@refack
Copy link
Contributor Author

refack commented Jul 6, 2017

FWIW, the llnode test suite generated core files using --abort-on-uncaught-exception, loads them into lldb, and executes commands. Since that project lives under the nodejs org, IMO that should be enough validation.

Great!

I'll see what can be done for Windows.

@Trott
Copy link
Member

Trott commented Jun 7, 2018

@refack Should this remain open?

@joyeecheung
Copy link
Member

joyeecheung commented Jun 8, 2018

FWIW I don't see the point of testing core dump generation. The flag triggers a fatal error, whether that leads to a core dump being written to disk and whether the core dump is properly generated depend on the configuration of the system. If there is a bug in the core dump generation, it's unlikely that we can fix it here.

@gireeshpunathil
Copy link
Member

agreed - lot of dependancies on user limits, OS limits and file system space. Plus I never came across a user problem in terms of the flag not working. I suggest closing and revisiting if it is an issue.

@Trott Trott closed this as completed Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

6 participants