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

Trim runtest #89240

Closed
wants to merge 3 commits into from
Closed

Conversation

Nicholas-Baron
Copy link
Contributor

@Nicholas-Baron Nicholas-Baron commented Sep 25, 2021

Work towards #60302.

src/tools/compiletest/src/runtest.rs was over 4k lines. To break it down into smaller chunks,

  1. The TestCx struct was extracted, as it constituted the bulk of the file.
  2. The functions following the pattern run_foo_test were moved into a separate file, as they were ~1k lines by themselves.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Sep 25, 2021

Along the way, runtest.rs was replaced with runtest/mod.rs.

Why this change? Rustc will happily accept runtest.rs with runtest/test_cx.rs as a submodule.

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Sep 25, 2021
@Nicholas-Baron
Copy link
Contributor Author

@jyn514 More a stylistic preference than anything else.

I will be redoing this PR as some smaller changes were unnecessary.

@jyn514
Copy link
Member

jyn514 commented Sep 25, 2021

@Nicholas-Baron it seems better to keep the original filename to avoid more churn than necessary.

@bors
Copy link
Contributor

bors commented Sep 26, 2021

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

@camsteffen
Copy link
Contributor

Rather than test_cx, I would like to see more semantically meaningful modules. For instance, there could be a module for constructing rustc args and a module for parsing output.

@Nicholas-Baron
Copy link
Contributor Author

@camsteffen While I agree with the sentiment, the TestCx struct seems to cover most of the semantics in the runtest module. I am unfamiliar with this part of the codebase, so taking the time to divide up the TestCx impl block semantically would be a bit longer than what I initially intended.

@Mark-Simulacrum
Copy link
Member

I think this is not quite the change I would expect for this goal. Generally speaking, just moving code to a separate file (particularly when the move is based on a somewhat arbitrary line) isn't really making it easier to understand the file I think. Instead, I would suggest some of these changes as a start, if you'd like to cleanup compiletest a little. I think these will lead to slightly better structure for the code, while keeping the bits that have a shared structure to them in the same place for easier reading.

  • 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 cleanup 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.

@Mark-Simulacrum Mark-Simulacrum 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 Oct 2, 2021
@Nicholas-Baron
Copy link
Contributor Author

@Mark-Simulacrum Thank you for the suggestions. I have spun them off as a "subtask", that will probably take multiple PRs to finish.

@Nicholas-Baron Nicholas-Baron deleted the trim_runtest branch October 2, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants