Skip to content

ui/lto: move and rename two tests from issues/#154167

Open
ujjwalvishwakarma2006 wants to merge 3 commits intorust-lang:mainfrom
ujjwalvishwakarma2006:move-lto-tests
Open

ui/lto: move and rename two tests from issues/#154167
ujjwalvishwakarma2006 wants to merge 3 commits intorust-lang:mainfrom
ujjwalvishwakarma2006:move-lto-tests

Conversation

@ujjwalvishwakarma2006
Copy link

@ujjwalvishwakarma2006 ujjwalvishwakarma2006 commented Mar 21, 2026

Move:

  • tests/ui/issues/issue-44056.rs -> tests/ui/lto/lto-avx-target-feature.rs
    • The first test enables both AVX target features and LTO but does not exercise AVX-specific behavior. The test primarily checks that LTO builds successfully with these flags; place it under ui/lto for consistency.
  • tests/ui/issues/issue-51947.rs -> tests/ui/lto/lto-weak-merge-functions.rs
    • The second test exercises weak linkage interacting with LTO (merge-functions), so it should belong in ui/lto.

I have also added a commented line at the top of each test file indicating the issue it originated from.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 21, 2026
@zedddie
Copy link
Contributor

zedddie commented Mar 21, 2026

hey! usually you'd split these into separate commits (check this discussion) and add a comment at the top referencing the original issue, something like //! regression test for <https://github.com/rust-lang/rust/issues/XXXXX>.

@Kivooeo Kivooeo self-assigned this Mar 21, 2026
@ujjwalvishwakarma2006
Copy link
Author

ujjwalvishwakarma2006 commented Mar 21, 2026

@zedddie Oh. Sorry, I wasn't aware of this. I thought I should consolidate as many files as possible into a single commit so that they don't clutter commits that actually fix some bugs or add features.

Also, isn't it inefficient? Because after every commit, the "internal tool" runs some checks, and it takes about an hour to execute those checks.

BTW, I have made changes in the top comment.

@zedddie
Copy link
Contributor

zedddie commented Mar 21, 2026

Oh. Sorry, I wasn't aware of this

It's totally fine! You aren't supposed to know everything beforehand.

Also, isn't it inefficient? Because after every commit, the "internal tool" runs some checks, and it takes about an hour to execute those checks.

I believe the tidy check should run as pre-push hook, not after each commit. I can suggest you looking at this chapter of rustc-dev-guide for more info!

@zedddie
Copy link
Contributor

zedddie commented Mar 21, 2026

BTW, I have made changes in the top comment.

Oops maybe what I said wasn't really clear, I meant adding this lines to the top of the renamed test files.
So for example test tests/ui/issues/issue-44056, after renaming would loss metadata about the issue it originates from, so you should add the line at the top of a test file:

//! regression test for <https://github.com/rust-lang/rust/issues/44056>
//@ build-pass (FIXME(55996): should be run on targets supporting avx)
//@ only-x86_64
//@ no-prefer-dynamic
//@ compile-flags: -Ctarget-feature=+avx -Clto
//@ ignore-backends: gcc

fn main() {}

not PR description.
You can check this PR to have a picture of how workflow is done usually.

@ujjwalvishwakarma2006
Copy link
Author

@zedddie
I have made the changes following the steps mentioned in the discussion. Kindly review.

@ujjwalvishwakarma2006 ujjwalvishwakarma2006 marked this pull request as ready for review March 21, 2026 19:48
Copilot AI review requested due to automatic review settings March 21, 2026 19:48
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2026

This comment was marked as spam.

@Kivooeo
Copy link
Member

Kivooeo commented Mar 21, 2026

FYI: We're discouraging using Copilot for reviews. Once IT completes its review, Copilot-generated messages will be hidden as spam.

@ujjwalvishwakarma2006
Copy link
Author

ujjwalvishwakarma2006 commented Mar 21, 2026

FYI: We're discouraging using Copilot for reviews. Once IT completes its review, Copilot-generated messages will be hidden as spam.

I don't know why the Copilot review started automatically. Who is responsible for changing this functionality - the contributor or the organization? Do I need to disable the Copilot reviewer?

If yes, let me fix this in future commits.

Edit: I got the answer, it's in my hands ( :

@Kivooeo
Copy link
Member

Kivooeo commented Mar 21, 2026

Yep, this should be in your settings -- we've already disabled automatic Copilot reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants