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

Console error info #16897

Merged
merged 3 commits into from Aug 8, 2017
Merged

Console error info #16897

merged 3 commits into from Aug 8, 2017

Conversation

Bjvanminnen
Copy link
Contributor

This attempts to add some better login for when we hit console errors.

(1) It includes the name of the test where the error is thrown, for example if I have

it('has an error', () => {
  console.error('brents');
});

I get

FAILED TESTS:
  unit tests
    ✖ "after each" hook for "has an error"
      Chrome 60.0.3112 (Mac OS X 10.11.6)
    Error: Error: Call to console.error from "has an error": brents
        at Context.<anonymous> (/Users/brent/git/cdo/apps/test/unit-tests.js:8536:16)

(2) It prints a stack trace from the error location:

START:
ERROR: '', 'brents'
LOG: 'Error
    at logStack (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:8494:12)
    at console.<anonymous> (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:8521:10)
    at Object.invoke (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:26675:33)
    at console.functionStub (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:26459:54)
    at Function.invoke (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:24463:52)
    at console.proxy [as error] (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:24370:23)
    at Context.<anonymous> (http://localhost:9876/base/test/unit-tests.js?c55b7fc58799c2620410407ab8a7ff3482416185:645185:14)
    at callFn (http://localhost:9876/base/node_modules/mocha/mocha.js?bc2b9e2e7b386b405368c9247d419c9a1d2718c8:4445:21)
    at Test.Runnable.run (http://localhost:9876/base/node_modules/mocha/mocha.js?bc2b9e2e7b386b405368c9247d419c9a1d2718c8:4437:7)
    at Runner.runTest (http://localhost:9876/base/node_modules/mocha/mocha.js?bc2b9e2e7b386b405368c9247d419c9a1d2718c8:4934:10)'
  unit tests
    progressReduxTest
      ✔ has an error
    ✖ "after each" hook for "has an error"

This won't fix the intermittent errors we've been seeing, but will hopefully make tracking them down easier.

@Bjvanminnen
Copy link
Contributor Author

This passed on my first test run (I was kind of hoping it would fail to validate that the logging is useful). I think there may be value in merging this though, so that we have more logging for the next time someone hits this.

In the meantime, I'm going to rerun and see if I can get a failure.

@islemaster
Copy link
Contributor

Awesome! I'm having a very hard time tracking down some errors in the React upgrade right now (I think they're from dependencies) and this will be super helpful.

sinon.stub(console, 'error').callsFake(msg => {
const prefix = throwingOnErrors ? '' : '[ignoring]';
console.error.wrappedMethod(prefix, msg);
if (throwingOnErrors) {
logStack();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're printing the stack right away, instead of storing the it and printing it with the stored error after the test? In fact, why not attach the stack to our firstError object?

Just suggestions, this is clearly an improvement without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, probably not any good reason. I'll try moving it.

@Bjvanminnen Bjvanminnen merged commit 40481ab into staging Aug 8, 2017
}

// TODO: temporary logging
console.log('is ErrorEvent: ', msg instanceof ErrorEvent);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i should get rid of this

@Bjvanminnen Bjvanminnen deleted the consoleErrorInfo branch August 8, 2017 16:07
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 this pull request may close these issues.

None yet

2 participants