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

fix: work around broken console methods #3085

Merged
merged 1 commit into from
Oct 5, 2018
Merged

fix: work around broken console methods #3085

merged 1 commit into from
Oct 5, 2018

Conversation

thorn0
Copy link
Contributor

@thorn0 thorn0 commented Jul 25, 2018

Sometimes native console methods are broken, but this shouldn't break tests.
It is especially undesirable in environments like Sauce Labs with limited access to the browser settings.
See angular/angular.js#16644 (comment)

context/karma.js Outdated
return Function.prototype.apply.call(orig, localConsole, arguments)
} catch (error) {
// Sometimes native console methods are broken, but this shouldn't break tests.
// It is especially undesirable in environments like Sauce Labs with limited access to the browser settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the three lines of comment. The info in the commit message is adequate.

context/karma.js Outdated
// Sometimes native console methods are broken, but this shouldn't break tests.
// It is especially undesirable in environments like Sauce Labs with limited access to the browser settings.
// See https://stackoverflow.com/questions/46913856/value-is-not-a-sequence-safari-exception
self.log('warn', ['Native method console.' + method + ' is broken:', error])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use string literal but without attributing the throw to a cause:

`Console method ${method} threw: ${error}`

This issue can also arise if users overwrite console: it may not be a native method problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we can't use ES2015 there. If we do, it doesn't work in Phantom and other old browsers.

@thorn0 thorn0 closed this Jul 25, 2018
@thorn0 thorn0 reopened this Jul 25, 2018
@thorn0
Copy link
Contributor Author

thorn0 commented Jul 25, 2018

@johnjbarton Not sure what happened to Travis, but looks unrelated to this PR. Please have a look,

@johnjbarton
Copy link
Contributor

The error is PhantomJS 2.1.1 (Linux 0.0.0)]: Disconnected (1 times). We see this a lot. (Probably should switch to headless chrome). But since this PR is about error messages I'd rather get a clean CI.

@thorn0 thorn0 closed this Jul 25, 2018
@thorn0 thorn0 reopened this Jul 25, 2018
@johnjbarton
Copy link
Contributor

Let's see if #3087 helps here.

@johnjbarton
Copy link
Contributor

Please sync to master and rebase. I think the longer timeout will allow CI to pass.

@thorn0 thorn0 closed this Jul 26, 2018
@thorn0 thorn0 reopened this Jul 26, 2018
@johnjbarton
Copy link
Contributor

closing and opening to trigger travis

@johnjbarton johnjbarton reopened this Jul 27, 2018
@thorn0 thorn0 closed this Aug 3, 2018
@thorn0 thorn0 reopened this Aug 3, 2018
@thorn0 thorn0 changed the title fix: work around broken native console methods fix: work around broken console methods Sep 9, 2018
Sometimes console methods are broken, but this shouldn't break tests.
See angular/angular.js#16644 (comment)
@thorn0
Copy link
Contributor Author

thorn0 commented Sep 9, 2018

@johnjbarton Travis is green finally. It was failing because I tried to use ES2015 (template literals) in context/karma.js, which gets to browsers without down-level transpilation. That's why it didn't work in Phantom.

@lusarz lusarz merged commit 873e4f9 into karma-runner:master Oct 5, 2018
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.

3 participants