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

Enable logging with verbose flag #15657

Merged
merged 2 commits into from May 31, 2018
Merged

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented May 30, 2018

This fixes the logging to terminal feature as well as enables logging to the browser console.
I believe the documentation for -verbose is sufficient already.

Adding -verbose could lead to failure to throw test errors if there's console.log/warn/info involved, but I think that's really minor.

I am also open to create a -debug flag and separate the log to terminal vs log to console feature.

@zhouyx zhouyx requested a review from rsimha May 30, 2018 02:05
@@ -208,6 +208,7 @@ module.exports = {
failOnConsoleError: !process.env.TRAVIS && !process.env.LOCAL_PR_CHECK,
// TODO(rsimha, #14432): Set to false after all tests are fixed.
captureConsole: true,
debugConsole: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this verboseLogging, since it's triggered by the --verbose flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. renamed

const {debugConsole} = window.__karma__.config;
if (debugConsole) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the logic here will result in consoleInfoLogWarnSandbox unnecessarily being created. You could move this to where stubConsoleInfoLogWarn() is called in beforeEach().

amphtml/test/_init_tests.js

Lines 376 to 384 in cbf3929

beforeEach(function() {
this.timeout(BEFORE_AFTER_TIMEOUT);
beforeTest();
testName = this.currentTest.fullTitle();
stubConsoleInfoLogWarn();
warnForConsoleError();
initialGlobalState = Object.keys(global);
initialWindowState = Object.keys(window);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Cool!

@rsimha
Copy link
Contributor

rsimha commented May 30, 2018

/cc @calebcordry

@zhouyx zhouyx merged commit 2331e0d into ampproject:master May 31, 2018
const {verboseLogging} = window.__karma__.config;
if (!verboseLogging) {
stubConsoleInfoLogWarn();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we're no longer creating the sandbox when --verbose is used, resulting in the following error:

  TypeError: Cannot read property 'restore' of undefined
      at restoreConsoleInfoLogWarn (/tmp/test/_init_tests.js:373:28 <- /tmp/20fafa07a5f318252b366d0a8daf1740.browserify:72980:29)
      at Context.<anonymous> (/tmp/test/_init_tests.js:410:2 <- /tmp/20fafa07a5f318252b366d0a8daf1740.browserify:73018:3)

Copy link
Contributor

Choose a reason for hiding this comment

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

To fix, we'd need to include this change too:

// Used to restore info, log, and warn level logging after each test.
function restoreConsoleInfoLogWarn() {
  if (consoleInfoLogWarnSandbox) {
    consoleInfoLogWarnSandbox.restore();
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants