Fix codegen-units tests that were disabled 8 years ago #128929

saethlin · 2024-08-10T16:27:32Z

I don't know if any of these tests still have value. They were disabled by #33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.

rustbot · 2024-08-10T16:27:40Z

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

compiler-errors · 2024-08-10T16:39:15Z

Yep, doesn't hurt to have these

r=me when green

rustbot · 2024-08-10T17:33:09Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

saethlin · 2024-08-10T19:47:26Z

@compiler-errors @jieyouxu I've added new normalization logic to compiletest to get the tests to pass. I'd appreciate a re-review.

compiler-errors

Kinda a hack but fine I guess. I wonder if we should be normalizing the test path in general in //~ strings -- IIRC we don't do it for diagnostics so people usually have to write //~ ERROR expected {closure@ and leave out the path part...

compiler-errors · 2024-08-10T21:30:37Z

@bors r+ rollup

bors · 2024-08-10T21:30:39Z

📌 Commit 03b6c2f has been approved by compiler-errors

It is now in the queue for this repository.

jieyouxu · 2024-08-11T01:45:26Z

src/tools/compiletest/src/runtest.rs

+            .map(|line| {
+                line.replace(&self.testpaths.file.display().to_string(), "TEST_PATH").to_string()
+            })


Discussion (not blocking): I think this is fine for codegen-units tests, but ui tests normalize paths in their output with placeholders like $SRC_DIR, $DIR, $COMPILER_DIR. Should we follow that for consistency?

rust/src/tools/compiletest/src/runtest.rs

Lines 4322 to 4328 in 730d5d4

let base_dir = Path::new("/rustc/FAKE_PREFIX");

// Paths into the libstd/libcore

normalize_path(&base_dir.join("library"), "$SRC_DIR");

// `ui-fulldeps` tests can show paths to the compiler source when testing macros from

// `rustc_macros`

// eg. /home/user/rust/compiler

normalize_path(&base_dir.join("compiler"), "$COMPILER_DIR");

But AFAIK those are only applied when the diagnostic is --format json or something. We probably also have other ways to do normalization for different test suites, it's a bit of a YOLO situation in here.

(unrelated remark I need to breakup / tidy up runtest.rs because the 4700 lines of run test logic is a pain every time I try to look up something)

Yeah. We should probably apply all the path normalizations to all tests to be honest. It can be hard to remember how little code sharing there is between test suites.

jieyouxu

Thank you for trying to fix those tests!

…s, r=compiler-errors Fix codegen-units tests that were disabled 8 years ago I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them. I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.

Rollup of 5 pull requests Successful merges: - rust-lang#128643 (Refactor `powerpc64` call ABI handling) - rust-lang#128873 (Add windows-targets crate to std's sysroot) - rust-lang#128916 (Tidy up `dump-ice-to-disk` and make assertion failures dump ICE messages) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`) r? `@ghost` `@rustbot` modify labels: rollup

…s, r=compiler-errors Fix codegen-units tests that were disabled 8 years ago I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them. I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.

…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#126245 (Greatly speed up doctests by compiling compatible doctests in one file) - rust-lang#128394 (Unify run button display with "copy code" button and with mdbook buttons) - rust-lang#128878 (Slightly refactor `Flags` in bootstrap) - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128967 (std::fs::get_path freebsd update.) - rust-lang#128978 (Use `assert_matches` around the compiler more) r? `@ghost` `@rustbot` modify labels: rollup

…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#128149 (nontemporal_store: make sure that the intrinsic is truly just a hint) - rust-lang#128394 (Unify run button display with "copy code" button and with mdbook buttons) - rust-lang#128537 (const vector passed through to codegen) - rust-lang#128632 (std: do not overwrite style in `get_backtrace_style`) - rust-lang#128878 (Slightly refactor `Flags` in bootstrap) - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`) - rust-lang#128978 (Use `assert_matches` around the compiler more) - rust-lang#128994 (Fix bug in `Parser::look_ahead`.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128929 - saethlin:enable-codegen-units-tests, r=compiler-errors Fix codegen-units tests that were disabled 8 years ago I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them. I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.

rustbot assigned Mark-Simulacrum Aug 10, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2024

compiler-errors assigned compiler-errors and unassigned Mark-Simulacrum Aug 10, 2024

This comment has been minimized.

Sign in to view

saethlin force-pushed the enable-codegen-units-tests branch from abada33 to 1e4a02e Compare August 10, 2024 17:33

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 10, 2024

This comment has been minimized.

Sign in to view

Fix and enable disabled codegen-units tests

03b6c2f

saethlin force-pushed the enable-codegen-units-tests branch from 1e4a02e to 03b6c2f Compare August 10, 2024 18:03

compiler-errors approved these changes Aug 10, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2024

jieyouxu reviewed Aug 11, 2024

View reviewed changes

jieyouxu mentioned this pull request Aug 11, 2024

compiletest: test suites handle normalizations differently and inconsistently #128952

Open

jieyouxu approved these changes Aug 11, 2024

View reviewed changes

jieyouxu mentioned this pull request Aug 11, 2024

Rollup of 5 pull requests #128966

Closed

jieyouxu mentioned this pull request Aug 11, 2024

Rollup of 5 pull requests #128968

Closed

GuillaumeGomez mentioned this pull request Aug 12, 2024

Rollup of 7 pull requests #129006

Closed

GuillaumeGomez mentioned this pull request Aug 12, 2024

Rollup of 10 pull requests #129008

Merged

bors merged commit 5b6379a into rust-lang:master Aug 12, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 12, 2024

saethlin deleted the enable-codegen-units-tests branch August 12, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix codegen-units tests that were disabled 8 years ago #128929

Fix codegen-units tests that were disabled 8 years ago #128929

saethlin commented Aug 10, 2024 •

edited

Loading

rustbot commented Aug 10, 2024

compiler-errors commented Aug 10, 2024

This comment has been minimized.

rustbot commented Aug 10, 2024

This comment has been minimized.

saethlin commented Aug 10, 2024

compiler-errors left a comment

compiler-errors commented Aug 10, 2024

bors commented Aug 10, 2024

jieyouxu Aug 11, 2024 •

edited

Loading

jieyouxu Aug 11, 2024 •

edited

Loading

saethlin Aug 11, 2024

jieyouxu Aug 11, 2024

jieyouxu left a comment

	let base_dir = Path::new("/rustc/FAKE_PREFIX");
	// Paths into the libstd/libcore
	normalize_path(&base_dir.join("library"), "$SRC_DIR");
	// `ui-fulldeps` tests can show paths to the compiler source when testing macros from
	// `rustc_macros`
	// eg. /home/user/rust/compiler
	normalize_path(&base_dir.join("compiler"), "$COMPILER_DIR");

Fix codegen-units tests that were disabled 8 years ago #128929

Fix codegen-units tests that were disabled 8 years ago #128929

Conversation

saethlin commented Aug 10, 2024 • edited Loading

rustbot commented Aug 10, 2024

compiler-errors commented Aug 10, 2024

This comment has been minimized.

rustbot commented Aug 10, 2024

This comment has been minimized.

saethlin commented Aug 10, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors commented Aug 10, 2024

bors commented Aug 10, 2024

jieyouxu Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

saethlin Aug 11, 2024

Choose a reason for hiding this comment

jieyouxu Aug 11, 2024

Choose a reason for hiding this comment

jieyouxu left a comment

Choose a reason for hiding this comment

saethlin commented Aug 10, 2024 •

edited

Loading

jieyouxu Aug 11, 2024 •

edited

Loading

jieyouxu Aug 11, 2024 •

edited

Loading