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

Make WebDriver reporter more resilient against DOM-nuking tests when writeHtml is true #435

Closed
kitsonk opened this issue Jul 13, 2015 · 6 comments
Labels
effort-low Should be pretty quick enhancement A new or improved feature

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Jul 13, 2015

I am getting the following error when running certain tests under runner:

Reporter error!
TypeError: Unable to get property 'parentNode' of undefined or null reference
at suiteEnd  <__intern/lib/reporters/WebDriver.js:51:5>
at Anonymous function  <__intern/lib/ReporterManager.js:229:7>
at emit  <__intern/lib/ReporterManager.js:211:4>
at report  <__intern/lib/Suite.js:199:6>
at end  <__intern/lib/Suite.js:193:5>
at Anonymous function  <__intern/node_modules/dojo/Promise.js:156:33>
at runCallbacks  <__intern/node_modules/dojo/Promise.js:19:13>
at Anonymous function  <__intern/node_modules/dojo/Promise.js:103:21>
at run  <__intern/node_modules/dojo/Promise.js:51:33>reset
``` I suspect this is because I am modifying the DOM myself and the WebDriver reporting is getting a bit lost and is accessing the elements in an "unsafe" way.

In particular, wouldn't [this line](https://github.com/theintern/intern/blob/master/lib/reporters/WebDriver.js#L51):

```js
this.suiteNode = this.suiteNode.parentNode.parentNode || document.body;
``` Be more safely written as:

```js
this.suiteNode = this.suiteNode && this.suiteNode.parentNode && this.suiteNode.parentNode.parentNode || document.body;
@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 13, 2015

I tried the above in a personal branch and it resolves the issue where I am running tests the cause the reference to this.suiteNode to be lost.

@csnover
Copy link
Member

csnover commented Jul 29, 2015

I presume you are wiping everything out of document.body to cause this? The “safer” access prevents a crash but it doesn’t do much else good, it just causes the reporter to ignore the fact that this.suiteNode lost its parent and retargets it to a wrong destination (document.body).

The more correct solution for your case is probably to set runnerClientReporter.writeHtml to false in your config (in master only at the moment). A more correct patch for the reporter would probably be to write everything starting rooted at an anonymous container node instead of document.body, and then if someone blows up document.body it can be restored because a reference to the container will still be held and it can just be dumped back into the body.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 29, 2015

I presume you are wiping everything out of document.body to cause this?

Correct.

The more correct solution for your case is probably to set runnerClientReporter.writeHtml to false in your config (in master only at the moment).

Yes, fair enough.

A more correct patch for the reporter would probably be to write everything starting rooted at an anonymous container node instead of document.body, and then if someone blows up document.body it can be restored because a reference to the container will still be held and it can just be dumped back into the body.

Ok, let me work on that.

@csnover csnover changed the title Issue with using runner and WebDriver reporter Make WebDriver reporter more resilient against DOM-nuking tests when writeHtml is true Jul 31, 2015
@csnover csnover added enhancement A new or improved feature help-wanted effort-low Should be pretty quick labels Jul 31, 2015
@devpaul
Copy link
Contributor

devpaul commented Oct 3, 2016

I just experienced this issue while testing A-Frame since A-Frame replaces document.body there was no parentNode during SuiteEnd.

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 3, 2016

I will rebase #451 again! 😆

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 21, 2017

Fixed via #451 being merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-low Should be pretty quick enhancement A new or improved feature
Projects
None yet
Development

No branches or pull requests

3 participants