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

coverage: Replace color terminal tests with HTML output tests #122631

Closed
wants to merge 1 commit into from

Conversation

Zalathar
Copy link
Contributor

The main goal of these tests is to prevent #119033 from regressing. We can't do that with the usual plain text coverage reports, because they don't use column numbers to slice source code. So we need to emit either text reports with colour, or HTML reports.

Coloured text reports turn out to have two main problems:

  • There's no way to tell llvm-cov to emit ANSI escape sequences on Windows, which means we have to ignore the tests on Windows.
  • The output files contain raw ANSI escape sequences, making them awkward to read.
    • We also can't just render them to SVG, as some other tests do, due to rendering issues with full-width characters.

We can avoid those problems by switching to HTML reports instead. The tradeoffs are:

  • Extra code for normalizing HTML output.
  • The HTML snapshots have a lot more text in them, though with a few line breaks inserted they're still fairly readable.

@rustbot label +A-code-coverage

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 17, 2024
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, though things are still extremely hard to read in the snapshot output.

@Zalathar
Copy link
Contributor Author

Zalathar commented Mar 17, 2024

Yeah, my thinking is that if it's going to be somewhat unreadable either way, I'd prefer the kind of unreadable that works on Windows, and can at least be manually inspected with a text editor and web browser.

I can also play around with adding even more line breaks (to make the code stand out more), though I'm not sure how much difference that will make.

(One of my constraints is that I ultimately want the modified HTML output to still work and resemble the real thing, which is why I haven't just removed the gunk.)

@Zalathar
Copy link
Contributor Author

I've also stumbled on a different reason to want HTML output tests: the text and HTML reporters have subtly different output in some cases.

See taiki-e/cargo-llvm-cov#324 (comment) for an example. In text reports, the implied else arm is indicated by a ^ on the following line. In the HTML report it's present in the markup, but completely invisible.

(In the short term, I plan to work around this by making zero-width spans expand backwards instead of forwards. Eventually I'd like to get branch coverage into a state where I feel good about discarding zero-width spans completely.)

@Mark-Simulacrum
Copy link
Member

I don't have a strong opinion here. It looks like @oli-obk has been reviewing patches in this area, so r? @oli-obk

This seems fine on a surface level though.

@rustbot rustbot assigned oli-obk and unassigned Mark-Simulacrum Mar 30, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2024

  • We also can't just render them to SVG, as some other tests do, due to rendering issues with full-width characters.

Do you have an example of where this is an issue?

@Zalathar
Copy link
Contributor Author

Do you have an example of where this is an issue?

When I tried the terminal SVG renderer out a few weeks ago, any output with East Asian Wide characters (ひらがな, 漢字 etc) would end up rendering with the background colours misaligned from the actual text.

(IIRC, this was due to the fact that the terminal SVG renderer doesn't actually apply a “background colour” at the SVG level; it just creates background rectangles and then tries to align the text on top of it. That happens to work for fixed-width fonts with known metrics, but goes out of alignment whenever font fallback causes the SVG-handling code to have no idea how big the displayed characters are going to be.)

@Zalathar
Copy link
Contributor Author

And as mentioned above, there are cases where llvm-cov produces subtly different output for text reports vs HTML reports (specifically involving regions extending beyond the end of a line) that make me want to be able to specifically test HTML output anyway.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2024

Then we have the wide char issues with regular diagnostics, too.

I don't think it's good to have an unreviewable (on github) format. I'd like to use svg and just avoid wide chars in color tests

@Zalathar
Copy link
Contributor Author

I could try to modify the tests to use different Unicode characters to work around anstyle-svg limitations, but then we still wouldn't be able to run/bless them on Windows (because llvm-cov has no flag to force ANSI escapes on Windows).

@oli-obk
Copy link
Contributor

oli-obk commented Mar 31, 2024

Which parts of the HTML output do you actually care about? Can we strip most of it or use filecheck instead?

@Zalathar
Copy link
Contributor Author

The main things we care about are:

  • Making sure the text didn't accidentally get mangled (e.g. by slicing in the middle of a UTF-8 code point)
  • Making sure the regions of text that are highlighted are the ones that we expect to be highlighted

(That second point is one of the reasons why I abandoned my earlier efforts with anstyle-svg. If the rendered output can't be trusted to actually reflect the highlighted regions, then it loses a lot of its value.)

When I was preparing this PR, I made the decision to post-process the HTML as little as possible, so that I could open the actual HTML files in a browser and make sure the results looked sensible (and also inspect the actual markup if necessary).

But if you think direct GitHub reviewability is more important, there should be a lot of scope to post-process the HTML and slash it down into something that can realistically be inspected in a PR context. It would basically be plain text with a little bit of markup indicating colour-highlighted spans.

(The end result would be very close to running the colour-terminal tests, and then replacing ANSI escape codes with ad-hoc markup. Except that we'd be able to run the stripped-HTML tests on Windows.)

@oli-obk
Copy link
Contributor

oli-obk commented Mar 31, 2024

Thanks for the thorough explanation.

The main issue I see now is that if we go with this PR, people will just bless and forget, while reviewers will mostly give it a glance to see that it didn't get totally messed up. But small important things will not be noticed. Without explicit annotations for what the specific test cares about, I fear they'll regress at some point.

We can just see how it goes in practice if you're alright with that risk

@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 1, 2024

@rustbot author

The above conversation has changed my priorities a bit, so I'm taking this off the review queue until I've had a chance to figure out how I want to proceed.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2024
@bors
Copy link
Contributor

bors commented May 20, 2024

☔ The latest upstream changes (presumably #125331) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@Zalathar any updates on this? thanks

@Zalathar
Copy link
Contributor Author

I don't intend to pursue this any time soon, so for now I'll just close it.

@Zalathar Zalathar closed this Aug 11, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants