Don't clobber window.console #199

patocallaghan · 2018-05-18T08:38:47Z

Since upgrading a JS framework (Ember 3.x) we use, Browserstack has failed to run our tests correctly, throwing exceptions and causing test failures.

Internally the framework relies on the presence of window.console.info and window.console.debug but Browserstack clobbers window.console and only re-implements console.log and console.warn.

This change means we will always have all window.console methods, but console.warn and console.log are still overridden for the runner's purposes.

patocallaghan · 2018-05-18T08:40:34Z

lib/_patch/browserstack.js

@@ -38,7 +38,7 @@
  }

  if (typeof console !== 'object') {


As this code is being run inside an IIFE, I fail to see how this conditional will ever be true? Won't it always be undefined?

@patocallaghan i fail to understand this statement. If console is always undefined this conditional will always be true. And I could not find any reference to say that the console object is undefined in an IIFE. Could you also provide a sample ember test which fails. It will help me debug better.

I didn't add that code so 🤷‍♂️ It appears it was added here c6c51c4 5 years ago 😄 FYI I'm running this on Node 8.9

I have these comments:

In the browser context window.console and console are the same object. So if console is not an object window.console wouldn't be either. So this change shouldn't really affect your tests.

If it does, I would suggest this change. All checks be made on an object browserstack_console = console || window.console. At the end window.console and console should be made equal to browserstack_console. This would take into account both console and window.console and then override the methods. It would make for a cleaner flow.
Also any sample ember test cases that fail?

Yep, generally window.console and console are the same in Browser-land but for some reason in this context they are not. Is there something on the Browserstack side clobbering console?

I've updated the logic to do as you say and added a browserstack_console variable to clean things up a bit.

Re:tests
You don't specifically need Ember tests to see this. I've added some console method asserts to browserstack-runner/tests/behaviour/resources/qunit_test1.js. If you revert my fix you'll see that these functions appear as undefined in Browserstack. Screenshot below:

patocallaghan · 2018-05-18T09:48:03Z

It looks like the failing CI is unrelated to my change. Master is also failing with the same error https://travis-ci.org/browserstack/browserstack-runner/builds/360235624

 1) Config Assertions should run normally with valid config:
     Uncaught Error: Error getting browsers list from BrowserStack
      at /home/travis/build/browserstack/browserstack-runner/lib/configParser.js:16:15
      at ApiClient.<anonymous> (/home/travis/build/browserstack/browserstack-runner/node_modules/browserstack/lib/api.js:24:4)
      at IncomingMessage.<anonymous> (/home/travis/build/browserstack/browserstack-runner/node_modules/browserstack/lib/client.js:73:5)
      at IncomingMessage.emit (events.js:185:15)
      at endReadableNT (_stream_readable.js:1106:12)
      at process._tickCallback (internal/process/next_tick.js:114:19)

mohitmun · 2018-05-21T07:34:24Z

@patocallaghan This is helpful 👍. did you try running tests running locally? we are fixing travis failures. will get it merged once its fixed.

patocallaghan · 2018-05-21T08:11:22Z

@mohitmun Yes I did, with both npm test and npm run test-ci but it fails some tests with the same as errors as on CI Uncaught Error: Error getting browsers list from BrowserStack.

mohitmun · 2018-05-21T10:07:47Z

@patocallaghan When running tests, it tries to retrieve browsers list from API which needs authentication. must be failing because of no credentials. try exporting browserstack credentials into you environment.

export BROWSERSTACK_USERNAME=XXXX
export BROWSERSTACK_ACCESS_KEY=XXXX

Let me know if it works

patocallaghan · 2018-07-11T09:35:05Z

Sorry for the silence here. I ran the tests locally with my Browserstack credentials and everything looks fine. No failing tests.

patocallaghan · 2018-08-13T08:16:26Z

@mohitmun any update here?

patocallaghan · 2018-08-17T11:50:07Z

tests/behaviour/resources/qunit_test1.js

@@ -1,4 +1,13 @@
 QUnit.module('Partial Tests', function() {
+  QUnit.test('console Tests', function(assert) {


@ray-pankaj I've added these assertions so you can reproduce the problem. Just revert my fixes to see console.info fail

Everything looks good, but since you added unit test for console, Travis test fail when run locally as the number of tests have increased and that hasn't been changed. Please fix that. And just for sanity, could you also add {} to the initialisation of browserstack_console

In qunit_test1.js the unit tests written should have booleans in the assertion, for instance typeof console.info === "function" to make the spec pass, as assert.ok expects booleans.

Thanks @ray-pankaj I've updated per your feedback. For some reason I can't seem to test the full suite locally though. When I run npm run test-behaviour I get the following error for some of the tests:

Uncaught AssertionError [ERR_ASSERTION]: 'Error: BrowserStackLocal failed to launch within 30 seconds.' == null

Ran tests locally. All pass. Merging now.

patocallaghan · 2018-08-23T07:05:59Z

Thanks @ray-pankaj 🙇 Any idea when we'll see a release with the patch?

patocallaghan commented May 18, 2018

View reviewed changes

patocallaghan changed the title ~~Don't clobbber window.console~~ Don't clobber window.console May 18, 2018

patocallaghan force-pushed the patoc/console-clobber branch from 619b4f5 to 4268931 Compare August 17, 2018 11:47

patocallaghan commented Aug 17, 2018

View reviewed changes

vedharish approved these changes Aug 17, 2018

View reviewed changes

Don't clobbber window.console

826d2c7

patocallaghan force-pushed the patoc/console-clobber branch from 4268931 to 0d4b0b3 Compare August 20, 2018 12:04

Add asserts for console

ad0a2e6

patocallaghan force-pushed the patoc/console-clobber branch from 0d4b0b3 to ad0a2e6 Compare August 22, 2018 09:41

pankaj-a merged commit caea6e3 into browserstack:master Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't clobber window.console #199

Don't clobber window.console #199

patocallaghan commented May 18, 2018

patocallaghan May 18, 2018

pankaj-a Aug 14, 2018

patocallaghan Aug 14, 2018

pankaj-a Aug 16, 2018

patocallaghan Aug 17, 2018

patocallaghan commented May 18, 2018

mohitmun commented May 21, 2018

patocallaghan commented May 21, 2018 •

edited

mohitmun commented May 21, 2018

patocallaghan commented Jul 11, 2018

patocallaghan commented Aug 13, 2018

patocallaghan Aug 17, 2018

pankaj-a Aug 20, 2018

pankaj-a Aug 21, 2018 •

edited

patocallaghan Aug 22, 2018

pankaj-a Aug 22, 2018

patocallaghan commented Aug 23, 2018

		@@ -1,4 +1,13 @@
		QUnit.module('Partial Tests', function() {
		QUnit.test('console Tests', function(assert) {

Don't clobber window.console #199

Don't clobber window.console #199

Conversation

patocallaghan commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patocallaghan commented May 18, 2018

mohitmun commented May 21, 2018

patocallaghan commented May 21, 2018 • edited

mohitmun commented May 21, 2018

patocallaghan commented Jul 11, 2018

patocallaghan commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pankaj-a Aug 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patocallaghan commented Aug 23, 2018

patocallaghan commented May 21, 2018 •

edited

pankaj-a Aug 21, 2018 •

edited