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/html reporter #2728

Closed
wants to merge 6 commits into from
Closed

Fix/html reporter #2728

wants to merge 6 commits into from

Conversation

ScottFreeCode
Copy link
Contributor

@ScottFreeCode ScottFreeCode commented Mar 5, 2017

Fixes potential issues with unescaped text, including in the querystring used as part of the grep links.

Also fixes #2300 and fixes the HTML half of #2298

Fixing #2285 is tricky. Per #1464, we can't just take the pathname out. There are a lot of ways to manipulate this, but all of them have some weird problem involved... with one possible exception: using the href and just dropping the search/hash from it.

window.location.href.replace(new RegExp('^(.*?' + escapeRe(window.location.pathname) + ').*$'), '$1')
// or
window.location.href.replace(new RegExp('^(.*)' + escapeRe(window.location.search + window.location.hash) + '$'), '$1')

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.109% when pulling c1b6950 on fix/html-reporter into a2fc76c on master.

I can't believe I forgot the right string quote character. Javascript, man...
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 85.822% when pulling ce1415e on fix/html-reporter into a2fc76c on master.

@ScottFreeCode
Copy link
Contributor Author

What? No, seriously -- what? I change " to ' and coverage drops over one whole percent?? Is there some massive element of nondeterminism in play here or something?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.209% when pulling e23a1de on fix/html-reporter into acd7701 on master.

@boneskull
Copy link
Member

@ScottFreeCode looks like you just volunteered to write tests against the html reporter 😉

@ScottFreeCode
Copy link
Contributor Author

@boneskull

@ScottFreeCode looks like you just volunteered to write tests against the html reporter 😉

I just had a brainwave that I could do that fairly easily:

  • in a test file that is only run in Karma
  • have the tests create the <div id="mocha"> element (if one already exists, temporarily save it and change its ID)
  • then create a new instance of the HTML reporter with a dummy runner and emit events on the dummy runner
  • restore anything that was changed re. divs and IDs (this could be additionally put in an afterEach hook in case anything goes wrong during the preceding parts of the test)
  • assert on the temporary, test-created id="mocha" div having been correctly populated by the reporter

Preliminary experimentation indicates not only that this should be doable, but that it is way, way easier than any idea I've had for it in the past (e.g. using JSDOM or running multiple instances of Mocha somehow). I would actually like to finish this at some point...

The downside? I have no idea how to get a coverage report out of our browserifying Karma setup, and we are not currently set up to send Coveralls coverage reports from more than one CI job (although if we got that figured out, we could check coverage in a wider variety of environments and get rid of a lot of missed branches in the code that are handling different environments; e.g. the to-iso-string module is the other least covered module besides the HTML reporter, and it's uncovered because it's only used if date objects lack a built-in toISOString function; additional "although": it seems like this should be able to be set up either using the istanbul-combine command we already have or else using Coveralls' "parallel" support).

On the other hand, the fact that the HTML reporter is non-covered on two counts (lack of tests for it and lack of running in Node even if we had tests for it) strongly suggests that any affect this PR has on coverage is coincidental and/or non-meaningful.

(In fact, I'm not sure why its coverage counts aren't just a big flat bunch of 0s. Do we have somewhere that is loading, but not using, the file in Node somewhere in the tests?? It might be nice to get proper browser coverage and figure out for sure that there isn't already something trying to test this, before I get too thorough with my new tests...)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: HTML reporter grep links do not respect fgrep, handle existing ? wrong
3 participants