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

Add unit test for html reporter #3102

Closed
wants to merge 7 commits into from
Closed

Conversation

38elements
Copy link
Contributor

@38elements 38elements commented Nov 11, 2017

Description of the Change

This adds unit test for html reporter.
This is related to #3071 .

@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 94.419% when pulling 0ac9125 on 38elements:html into 1bb6b39 on mochajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 94.419% when pulling 2427383 on 38elements:html into 1bb6b39 on mochajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 94.419% when pulling a23a483 on 38elements:html into 1bb6b39 on mochajs:master.

@38elements
Copy link
Contributor Author

I am sorry.
I did not notice the assignee.

@38elements 38elements closed this Nov 13, 2017
@ScottFreeCode
Copy link
Contributor

Actually, I'm sorry I didn't reply to this sooner -- no need to worry that the issue's assigned (that just helps us track who's looking at it) or even that there's already work toward it, we're definitely open to alternative attempts and new contributions! While my preference is to ensure things are tested and coverage collected in all the "real" browsers Mocha supports, I still need to figure out whether that's even going to be doable with the recent upgrades relating to Mocha's test harness for browser usage. So, while I can't guarantee which we'd pick at this point, if you've already coded it up we may as well see how it compares. Would you mind keeping this open for further assessment (or, at least, make sure the fork/branch stays available)? Thanks! (And thanks for taking a shot at adding test for this, in any case!)

@38elements
Copy link
Contributor Author

38elements commented Nov 15, 2017

Hi, @ScottFreeCode.
Thank you for your reply and contribution for mocha.
I reopen this pull request and keep the branch and opening this.

@38elements 38elements reopened this Nov 15, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 94.419% when pulling a23a483 on 38elements:html into 1bb6b39 on mochajs:master.

@boneskull
Copy link
Contributor

a jsdom-based test is better than nothing imo.

@boneskull boneskull added area: browser browser-specific qa area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Dec 7, 2017
Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks for this. I think, however, we just want to make this part of our existing browser tests.

The browser tests should be running the HTML reporter, so in theory it should "just work" 😉 Please let me know if you run into problems.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling d4dd5e4 on 38elements:html into c7730a6 on mochajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling f396abc on 38elements:html into c7730a6 on mochajs:master.

@38elements
Copy link
Contributor Author

I changed this.:smile:

document.body.appendChild(mochaElem);
}

function sleep (fn, ms) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this in particular is going to be brittle / susceptible to race conditions. but this test suite is unlike anything else we have, so we don't have the tooling.

all of assertions involving markup in innerHTML are also going to be brittle, but again, need better tooling.

I'm going to grab this PR and test it against saucelabs' browsers. if it is flaky, we'll have to change it. but if not, we'll just merge and deal w/ it later--unless, of course, you feel like pulling in Nightwatch.

Copy link
Contributor Author

@38elements 38elements Jan 22, 2018

Choose a reason for hiding this comment

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

Thank you for reviewing.
Since I think I am not going to be able to achieve it,
Would you please grab this pull request ?

}

function sleep (fn, ms) {
return new Promise((resolve) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

IE11 breaks with these

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

the arrow functions need to be replaced

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

the HTML attributes won't necessarily show in the same order, e.g.:

<li class="test pass fast"><h2>title<span class="duration">100ms</span>
  <a class="replay" href="/context.html?grep=fullTitle"></a></h2>
  <pre style="display: none;"><code>body</code></pre>
</li>

vs

<li class="test pass fast"><h2>title<span class="duration">100ms</span>
  <a href="/context.html?grep=fullTitle" class="replay"></a></h2>
  <pre style="display: none;"><code>body</code></pre>
</li>

@38elements
Copy link
Contributor Author

Thank you for reviewing.
I will change this within 5 days.

@coveralls
Copy link

coveralls commented Jan 20, 2018

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling a9cd4fd on 38elements:html into 2e7e4c0 on mochajs:master.

@38elements
Copy link
Contributor Author

I replaced the arrow functions and the comparing HTML attributes.
I added es6-promise.
But, I did not test by IE11.

@Nokel81
Copy link

Nokel81 commented Mar 28, 2018

Have you managed to test on IE11 yet?

@boneskull boneskull force-pushed the master branch 2 times, most recently from 4547268 to 7613521 Compare May 4, 2018 16:47
@rugk
Copy link

rugk commented Aug 11, 2018

Any news here?

@rugk rugk mentioned this pull request Aug 11, 2018
@38elements 38elements closed this Aug 11, 2018
@rugk
Copy link

rugk commented Aug 11, 2018

@38elements So what?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants