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: use accurate test links in HTML reporter #5228

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

danny0838
Copy link
Contributor

@danny0838 danny0838 commented Oct 12, 2024

PR Checklist

Overview

Fix inaccurate suite/test links that may hit unrelated tests:

  • Add '^' and ' ' for a suite link so that a link for Case1 won't hit Case10 test1.
  • Add '^' and '$' for a test link so that a link for CSS won't hit should fix invalid CSS.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

I don't think they were 🙂. From that doc:

As maintainers looking at this PR, we don't know what bugs you're suggesting this PR will fix, why it fixes them, or what test cases would be needed to enforce that they stay fixed. I appreciate the drive to improve things in Mocha (thanks!) but please follow the contributing guide. Switching to a draft PR in the meantime.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft October 13, 2024 21:01
@danny0838
Copy link
Contributor Author

danny0838 commented Oct 14, 2024

It should already be quite clear in the Overview section as well as in the commit message: this PR is to fix inaccurate test links for the HTML reporter, such as clicking on a link for describe('Case1') would get a query that hits unwanted describe('Case10', function () { it('test1') }), or clicking on a link for it('CSS') would get a query that hits unwanted it('should fix invalid CSS').

As for tests: this is a UI-related change and I wonder how and what test you would like it to be? Look back the commit history, many previous changes about the reporter didn't have related tests, such as e263c7a. If you really want a test then I think it either has to be run manually, or you'd have to implement a whole ecosystem for headless-browser related tests.

- Add '^' and ' ' for a suite link so that a link for `Case1` won't hit `Case10 test1`.
- Add '^' and '$' for a test link so that a link for `CSS` won't hit `should fix invalid CSS`.
@danny0838 danny0838 marked this pull request as ready for review October 15, 2024 12:00
@JoshuaKGoldberg
Copy link
Member

Thanks for filing the issue + linking, this makes a lot more sense to me now. Agreed on the bug and marked as accepting PRs! 🚀

👍 on the HTML reporter not having existing tests, so it's fine to omit tests for it.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🚀 works great, thanks!

I also tried with emojis like 🍎 and it worked with them.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 68803b6 into mochajs:main Oct 29, 2024
105 of 106 checks passed
@danny0838 danny0838 deleted the accurate-link branch November 4, 2024 01:23
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.

🐛 Bug: suite/test links for HTML reporter may hit unrelated tests
2 participants