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

Escape test/suite title for re in html reporter #1698

Merged
merged 1 commit into from Jul 3, 2015
Merged

Escape test/suite title for re in html reporter #1698

merged 1 commit into from Jul 3, 2015

Conversation

benvinegar
Copy link
Contributor

Fixes #1687

@dasilvacontin
Copy link
Contributor

Hi @benvinegar! Could you provide tests that assert this change fixes the issue? Thanks!

@benvinegar
Copy link
Contributor Author

@dasilvacontin Where do you recommend adding that? I could introduce a new /reporter test for the Html reporter (there isn't one yet), and verify that makeUrl escapes the grep component.

@benvinegar
Copy link
Contributor Author

Or I could add a new test to test/browser/, similar to test/browser/grep.html, which clicks through generated grep links to verify they work (instead of manipulating window.location directly).

@jbnicolai
Copy link

@benvinegar sorry for the silence on our side. Great fix! I'd recommend the later (simply because it seems easier), but both would prove the fix so pick whichever you'd prefer.

@benvinegar
Copy link
Contributor Author

Okay. I've added a test that demonstrates that clicking through links w/ regex chars works correctly.

But, I'm not sure how to run this test as part of CI. I'm not sure if test/browser/grep.* runs as part of CI either.

Please advise.

@boneskull
Copy link
Member

But, I'm not sure how to run this test as part of CI. I'm not sure if test/browser/grep.* runs as part of CI either.

test/browser is omitted from CI. I don't have much experience with them, but looking over them briefly I don't think they have much value in the first place--especially since the only way you can assert they work is to eyeball them.

boneskull added a commit that referenced this pull request Jul 3, 2015
Escape test/suite title for re in html reporter
@boneskull boneskull merged commit f652414 into mochajs:master Jul 3, 2015
@boneskull
Copy link
Member

@benvinegar thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

html reporter link to specific test is broken on some special characters
4 participants