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.

Sign up for GitHub

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

Jump to bottom

Splitting up compiletest::runtest #89475

Closed
3 of 4 tasks
Nicholas-Baron opened this issue Oct 2, 2021 · 4 comments · Fixed by #130566
Closed
3 of 4 tasks

Splitting up compiletest::runtest #89475

Nicholas-Baron opened this issue Oct 2, 2021 · 4 comments · Fixed by #130566
Assignees
@jieyouxu
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nicholas-Baron
Copy link
Contributor

Nicholas-Baron commented Oct 2, 2021 •
edited
Loading

Subtask to #60302.

  • RunPassValgrind is practically useless (valgrind is not installed on CI, and the tests are just run in run-pass mode). The right approach here is likely to refactor it as a modifier atop the regular run-pass tests -- it looks like the code generally just mirrors the RunPass mode, other than setting the runtool to valgrind. That mode can be enabled in the configuration of the test with // use-valgrind or something like that.
  • Move diff computation out into a separate module (DiffLine, Mismatch, make_diff, ...)
  • Move read2_abbreviated to the read2.rs module
  • Move DebuggerCommands and their parsing out to a separate file, alongside check_debugger_output (which is similarly parsing code).

I suspect this set of changes is likely harder, and might not get to <3000 lines; there's likely more cleanup work possible, though. This module has grown pretty organically over the last ~10 years -- there's a lot of small bits that can be adjusted to make things better. There's probably also subtle inconsistencies between various functions that would likely be nice to clean up and unify, and filing PRs for those would be helpful -- likely some care is needed, perhaps even some small test updates, but overall a good goal.

You might also be interested in #40713, as a somewhat larger project.

Originally posted by @Mark-Simulacrum in #89240 (comment)
@Nicholas-Baron Nicholas-Baron mentioned this issue Oct 2, 2021
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 7, 2021
@GuillaumeGomez
Rollup merge of rust-lang#89477 - Nicholas-Baron:compute_diff_rs, r=M…
48548c9 
…ark-Simulacrum

Move items related to computing diffs to a separate file

Work towards rust-lang#89475.
This was referenced Oct 8, 2021
Merged
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 9, 2021
@matthiaskrgr
Rollup merge of rust-lang#89687 - Nicholas-Baron:move_read2_abbreviat…
45ff2fb 
…ed, r=Mark-Simulacrum

Move `read2_abbreviated` function into read2.rs

Work towards rust-lang#89475.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 9, 2021
@matthiaskrgr
Rollup merge of rust-lang#89687 - Nicholas-Baron:move_read2_abbreviat…
a06c664 
…ed, r=Mark-Simulacrum

Move `read2_abbreviated` function into read2.rs

Work towards rust-lang#89475.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 10, 2021
@matthiaskrgr
Rollup merge of rust-lang#89691 - Nicholas-Baron:debugger_commands, r…
1b8f7a9 
…=Mark-Simulacrum

Move `DebuggerCommands` and `check_debugger_output` to a separate module

Work towards rust-lang#89475.

As part of this move, the public functions were changed to return `Result`. This is so that the error handling that initially took `&self: TestCx` can still use that `TestCx`.
@Nicholas-Baron
Copy link
Contributor Author

Nicholas-Baron commented Oct 11, 2021 •
edited
Loading

@Mark-Simulacrum I have started reading fn run_rpass_test and fn run_valgrind_test. While they are mostly similar, there seem to be some key differences.

  1. Valgrind tests are hardcoded to EmitMetadata::No, while Rpass tests seem to possibly emit metadata.
  2. Valgrind changes the run tool just before executing the compiled test.

I can see simplifying run_valgrind_test to a call to run_rpass_test. However, I do not think that is what was originally intended.

@Nicholas-Baron
Copy link
Contributor Author

@rustbot label: +C-cleanup.

@rustbot rustbot added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Oct 11, 2021
@Mark-Simulacrum
Copy link
Member

I think there's room for unifying the two -- we already have some support for running tests under a wrapper for e.g. android testing, so I imagine we might be able to reuse parts of that perhaps.

@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 29, 2024
@Zalathar Zalathar mentioned this issue May 29, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 29, 2024
@matthiaskrgr
Rollup merge of rust-lang#125719 - Zalathar:run-coverage, r=jieyouxu
6ef3dd0 
Extract coverage-specific code out of `compiletest::runtest`

I had been vaguely intending to do this for a while, but seeing rust-lang#89475 on the compiletest dashboard inspired me to actually go and do it.

This moves a few hundred lines of coverage-specific code out of the main module, making navigation a bit easier. There is still a small amount of coverage-specific logic in broader functions in that module, since it can't easily be moved.

This is just cut-and-paste plus fixing visibility and imports, so no functional changes.

I also removed the unit test for anonymizing line numbers in MC/DC reports, as foreshadowed by the comment I wrote when adding it. That functionality is now adequately exercised by the actual snapshot tests for MC/DC coverage.

(Removing the test now avoids the need to move it, or to make the function it calls visible.)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 29, 2024
@rust-timer
Unrolled build for rust-lang#125719
4fecf0e 
Rollup merge of rust-lang#125719 - Zalathar:run-coverage, r=jieyouxu

Extract coverage-specific code out of `compiletest::runtest`

I had been vaguely intending to do this for a while, but seeing rust-lang#89475 on the compiletest dashboard inspired me to actually go and do it.

This moves a few hundred lines of coverage-specific code out of the main module, making navigation a bit easier. There is still a small amount of coverage-specific logic in broader functions in that module, since it can't easily be moved.

This is just cut-and-paste plus fixing visibility and imports, so no functional changes.

I also removed the unit test for anonymizing line numbers in MC/DC reports, as foreshadowed by the comment I wrote when adding it. That functionality is now adequately exercised by the actual snapshot tests for MC/DC coverage.

(Removing the test now avoids the need to move it, or to make the function it calls visible.)
@jieyouxu jieyouxu added the A-compiletest Area: The compiletest test runner label May 31, 2024
@jieyouxu jieyouxu self-assigned this Aug 20, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Aug 20, 2024 •
edited
Loading

I really need to make sure we split up runtest.rs, every time I revisit it the more cursed it becomes.

@jieyouxu jieyouxu mentioned this issue Aug 23, 2024
Open
8 tasks
This was referenced Sep 19, 2024
Open
Merged
@bors bors closed this as completed in 6a762c9 Sep 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2024
@rust-timer
Unrolled build for rust-lang#130566
197cff9 
Rollup merge of rust-lang#130566 - jieyouxu:breakup-runtest, r=compiler-errors

Break up compiletest `runtest.rs` into smaller helper modules

Previously compiletest's `runtest.rs` was a massive 4700 lines file that made reading and navigation very awkward. This PR breaks the `runtest.rs` file up into smaller helper modules, one for each test suite/mode.

> [!NOTE]
> This PR should not contain functional changes, it is intended to be mostly code motion to breakup `runtest.rs` into smaller helper modules to make it easier to digest.
>
> This PR intentionally does not neatly reorganize where all the methods on `TestCx` goes, that is intended for a follow-up PR. Some methods on `TestCx` do not need to be on `TestCx`. It also does not address a weirdness in valgrind, that is intended for a follow-up PR as well.

Part of a series of compiletest cleanups rust-lang#130565.

Fixes rust-lang#89475.

r? `@ghost` (I need to do a self-review pass first)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jieyouxu jieyouxu

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
compiletest maintenance and improvements
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Break up compiletest runtest.rs into smaller helper modules jieyouxu/rust
4 participants
@Mark-Simulacrum @Nicholas-Baron @jieyouxu @rustbot