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

Revert "Bump ui_test to 0.23" #12949

Closed
wants to merge 2 commits into from
Closed

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Jun 17, 2024

#12746 introduced very buggy behaviour (.fixed tests not being checked, not showing what went wrong... etc.), this commit reverts that and it should fix all those issues. We'll check again once ui_test 0.24.0 is available.

@rustbot
Copy link
Collaborator

rustbot commented Jun 17, 2024

r? @y21

rustbot has assigned @y21.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 17, 2024
@flip1995
Copy link
Member

Did you already report this issue to ui_test and will it be fixed with 0.24?

@blyxyas
Copy link
Member Author

blyxyas commented Jun 17, 2024

It seems to be this issue oli-obk/ui_test#238

@flip1995
Copy link
Member

Is the issue just that no summary is printed or that the files don't actually get checked, resulting in broken tests?

@xFrednet
Copy link
Member

cc @oli-obk

@xFrednet
Copy link
Member

Maybe oli-obk/ui_test#176 could also be related? There was a time, where ui_test blessed files automatically, which would prevent any errors

@blyxyas
Copy link
Member Author

blyxyas commented Jun 18, 2024

Just tested this, seems that while the testing is not shown on screen, if the .fixed file was different it would output an error.
It's still quite annoying that the screen doesn't refresh to show this, and that it isn't uncommon to not show output. After commenting it to Timo on #12830, he sent this screenshot about how it looks.

EDIT: On CI it seems like there aren't any issues, could you test the patch on your PC to check? Maybe it's just my workflow

Oh sorry, didn't see this edit. Huh... yeah, I'm seeing something similar. image

I don't think this is something caused by this PR though. It also seems to happen on master for me.

Anyway, I'll try taking a closer look at the changes in a bit

@flip1995
Copy link
Member

flip1995 commented Jul 1, 2024

Running UI tests a few times the last few days, I also noticed this bug. It goes away when running with blessing, I think. But since it is just a visual bug, I would rather not downgrade ui_test again. This would introduce multiple ui_test versions in rustc and it might make it harder to upgrade once 0.24 is released (with this potentially fixed).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 3, 2024

I have figured out what the .fixed issue is and am currently refactoring to fix it. Not sure yet what the issue is with the missing final report

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 7, 2024
@blyxyas
Copy link
Member Author

blyxyas commented Jul 7, 2024

?? This shouldn't have propagated here? I was just testing a test bot and using this branch as a test for a PR (in my Clippy fork.)
I'll close the PR, and let's see if the issue gets fixed on future versions.

@blyxyas blyxyas closed this Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants