flaky firefox tests with protractor about angular-hint HOT 15 CLOSED

It seems that in Firefox the use of ngHintDom throws this error:

Ignoring get or set of property that has [LenientThis] because the "this" object is incorrect. hint.js:384
"Error: too much recursion
patchOnePrototype/</desc.get@http://localhost:8080/dist/hint.js:343:1
angular.hint.onMessage@http://localhost:8080/e2e/util.js:3:3
logMessage@http://localhost:8080/dist/hint.js:261:3
[3]</</</</<@http://localhost:8080/dist/hint.js:221:11
patchOnePrototype/</desc.get@http://localhost:8080/dist/hint.js:344:17
angular.hint.onMessage@http://localhost:8080/e2e/util.js:3:3
logMessage@http://localhost:8080/dist/hint.js:261:3
[3]</</</</<@http://localhost:8080/dist/hint.js:221:11
patchOnePrototype/</desc.get@http://localhost:8080/dist/hint.js:344:17
angular.hint.onMessage@http://localhost:8080/e2e/util.js:3:3
logMessage@http://localhost:8080/dist/hint.js:261:3
[3]</</</</<@http://localhost:8080/dist/hint.js:221:11
patchOnePrototype/</desc.get@http://localhost:8080/dist/hint.js:344:17
angular.hint.onMessage@http://localhost:8080/e2e/util.js:3:3
logMessage@http://localhost:8080/dist/hint.js:261:3
[3]</</</</<@http://localhost:8080/dist/hint.js:221:11
patchOnePrototype/<"[…] angular.js:9937
too much recursion

from angular-hint.

Hmmm... can you investigate?

from angular-hint.

Yes, I am investigating.

from angular-hint.

Currently, I'm wondering if in Firefox ngHintDOM actually patches getting .innerHTML, and this queues a message when we try to collect messages with

angular.hint.onMessage = function(message) {
  log.innerHTML += message;
};

from angular-hint.

ohhhh that might be it

from angular-hint.

I think that is it. Changing to

angular.hint.onMessage = function(message) {
  log.innerHTML += message;
};

Avoids the issue. However, it seems like a more robust solution would be to remove the listener within the execution of the function passed to domInterceptor? Firefox also seems to detect some other manipulation of the DOM, which was possibly something we had been looking at a while ago in domInterceptor. For instance, running the test in Chrome I detect getElementById, but in Firefox I detect getElementById as well as the getter for childNodes and the getter for nodeName fifteen times.

from angular-hint.

Or I guess I could monkey patch.innerHTML since this is already sort of a hack? I guess it depends on what the end goal is here, and what we would normally aim to do in a .onMessage function.

from angular-hint.

In Firefox, Object.getOwnPropertyDescriptor(Element.prototype, 'innerHTML') returns a configurable property which allows us to set the get/set methods.

In Chrome, innerHTML is not defined on Element.prototype.

from angular-hint.

Ah, woops. So unpatching .innerHTML fixes the issue for Firefox, but brings back problems for the other browsers: https://travis-ci.org/angular/angular-hint/jobs/30778026

I will try to add conditional logic.

from angular-hint.

the plot thickens

from angular-hint.

The moral of the story is to check multiple browsers locally before causing Travis to fail loudly.

from angular-hint.

Okay, back to passing: https://travis-ci.org/angular/angular-hint/builds/30778809

To recap, the overall issue is that Firefox attaches more properties to Element.prototype. Hence, when hintDOM is operating in Firefox, .innerHTML, the function we are using to expose messages for protractor is patched and therefore ends up calling the logMessage function repeatedly. I looked into using a strategy other than setting .innerHTML, but protractor does not give fine grained control of the developer console, and I found that to be a flaky alternative. Therefore, I added a workaround for Firefox that sets .innerHTML back to its unpatched version. However, since this unpatched version does not exist on Element.prototype in Chrome, we have to be careful in adding a check that it is defined before trying to revert the patch. This seems like an okay solution for the current issue. However, I think there may be other 'leaky' hintDOM issues in Firefox. Although I do get a correct detection of the expected getElementById function in my protractor test, Firefox also repeatedly detects get:nodeName. I think this needs investigation in domInterceptor.

from angular-hint.

I think that Firefox is also detecting part of the controller's instantiation, specifically when jQlite gathers a list of nodes, hence the many get:nodeName. I'm wondering if the best solution here would be to simply reduce what functions we are patching. Specifically these issues arise because Chrome and Safari do not allow patching of most get/set methods while Firefox does. Would it be effective to remove the patching of get/set patching in domInterceptor? Or is it a good thing that we could catch more use of DOM APIs only in Firefox?

from angular-hint.

Okay, after fixing my well-hidden cross browser consistency error in ngHintDom/dom-interceptor that allowed extra patching in Firefox angular/dom-interceptor#13, I can remove the hack in util.js that patched innerHTML back to its original state in Firefox.

from angular-hint.

Nicely done.

from angular-hint.