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

Crash in rule creator suppressed in `npm test` #5381

Closed
btmills opened this Issue Feb 23, 2016 · 4 comments

Comments

Projects
None yet
5 participants
@btmills
Member

btmills commented Feb 23, 2016

This was discovered in analysis of a failure in #5349.

In latest master (2bd57b6) on any version of Node, if:

  • A rule creator throws an exception (e.g. while parsing options)
  • That code path is one not used in ESLint's own config (e.g. max-statements, which we don't use at all)

...then npm test won't show any information about the failure:

> eslint@2.2.0 test /Users/brandon/code/eslint/eslint
> node Makefile.js test

Validating Makefile.js
Validating JSON Files
Validating Markdown Files
Validating JavaScript files
Validating JavaScript test files
Validating rules

  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬]=============================================================================
Writing coverage object [/Users/brandon/code/eslint/eslint/coverage/coverage.json]
Writing coverage reports at [/Users/brandon/code/eslint/eslint/coverage]
=============================================================================


  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․

  215 passing (560ms)

npm ERR! Test failed.  See above for more details.

You can try this yourself with the following diff, which will throw an error silently when you run npm test:

diff --git a/lib/rules/max-statements.js b/lib/rules/max-statements.js
index 78de52e..1e37c30 100644
--- a/lib/rules/max-statements.js
+++ b/lib/rules/max-statements.js
@@ -12,6 +12,8 @@

 module.exports = function(context) {

+    process.thisDoesntExist();
+
     //--------------------------------------------------------------------------
     // Helpers
     //--------------------------------------------------------------------------

If the error is thrown by a configuration that ESLint itself uses, then the stack trace will be shown as part of the "Validating JavaScript files" step. Applying the above diff to eqeqeq, for example, would fail with a stack trace before it ever got to the rule tests.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 25, 2016

Member

Yikes! Is something overwriting console.log? It looks like some output is missing.

Member

nzakas commented Feb 25, 2016

Yikes! Is something overwriting console.log? It looks like some output is missing.

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Feb 25, 2016

Member

I found: config-initializer.js#L221
console.log was overwritten in before hook, then crashed, then beforeEach hook isn't called.

Member

mysticatea commented Feb 25, 2016

I found: config-initializer.js#L221
console.log was overwritten in before hook, then crashed, then beforeEach hook isn't called.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 25, 2016

Member

@mysticatea nice catch! @IanVS do you have time to look at that?

Member

nzakas commented Feb 25, 2016

@mysticatea nice catch! @IanVS do you have time to look at that?

@IanVS

This comment has been minimized.

Show comment
Hide comment
@IanVS

IanVS Feb 25, 2016

Member

I was afraid it might have been something I did. 😞 Problem is, I don't really know a way to solve this. I'll try again tonight but if anyone has any ideas I'd love to hear them.

Member

IanVS commented Feb 25, 2016

I was afraid it might have been something I did. 😞 Problem is, I don't really know a way to solve this. I'll try again tonight but if anyone has any ideas I'd love to hear them.

IanVS added a commit that referenced this issue Feb 26, 2016

@btmills btmills closed this in 78f7ca9 Feb 26, 2016

btmills added a commit that referenced this issue Feb 26, 2016

Merge pull request #5404 from eslint/issue5381
Fix: Prevent crash from swallowing console.log (fixes #5381)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.