Skip to content

Conversation

GuillaumeGomez
Copy link
Member

As discussed on zulip, here is the very start of adding this new GUI testsuite.

r? @flip1995

changelog: Add new GUI testsuite

@rustbot
Copy link
Collaborator

rustbot commented Sep 8, 2025

flip1995 is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 8, 2025
@GuillaumeGomez GuillaumeGomez force-pushed the gui-testsuite branch 4 times, most recently from aab5162 to a7cb119 Compare September 8, 2025 21:54
@GuillaumeGomez
Copy link
Member Author

CI passed! \o/

@GuillaumeGomez
Copy link
Member Author

If they're on holiday let's pick someone else then. :)

r? @samueltardieu

@rustbot rustbot assigned samueltardieu and unassigned flip1995 Sep 9, 2025
@samueltardieu
Copy link
Member

I'd like to help, but I'm probably the worst person to select here as I really abhor everything related to GUI.
r? clippy

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@GuillaumeGomez
Copy link
Member Author

Applied suggestions.

@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 from the author. (Use `@rustbot ready` to update this status) labels Sep 24, 2025
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how if browser-ui-test isn't installed, the test is skipped, could we also do something like that on npm?

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 26, 2025
@GuillaumeGomez
Copy link
Member Author

I like how if browser-ui-test isn't installed, the test is skipped, could we also do something like that on npm?

Sure!

@GuillaumeGomez GuillaumeGomez force-pushed the gui-testsuite branch 2 times, most recently from 22997d2 to 1e4d6ff Compare September 26, 2025 15:52
@GuillaumeGomez
Copy link
Member Author

Fixed dogfood as well. ^^'

@GuillaumeGomez GuillaumeGomez requested a review from blyxyas October 3, 2025 12:57
@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 from the author. (Use `@rustbot ready` to update this status) labels Oct 3, 2025
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final comment before asking the team

View changes since this review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question before askign the team for approval, does goml have a system to output on error for each test?
Some reviewers really don't like GUI-related things, so having a custom message be outputed if a specific assert fails would be great.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error output looks like this: https://github.com/GuillaumeGomez/browser-UI-test/blob/master/tests/ui/assert-css-color.output, so normally you have all needed information (ie, file and line and even compared values).

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'll cc @rust-lang/clippy and maybe cc @rust-lang/clippy-contributors if they have feedback. While sadly having it be screenshot-based would load a huge dependency into our tree, the error messages are rich enough that contributors will be able to fix the issues themselves.

View changes since this review

@GuillaumeGomez
Copy link
Member Author

Applied suggestions.

@Alexendoo
Copy link
Member

To summarize my position from the zulip thread:

  • I think the maintenance burden outweighs the benefit of this style of test
  • It's annoying that I would have to use a windows machine to run the tests since both the rust dev environment and my personal one are on ARM Linux which is unsupported

While sadly having it be screenshot-based would load a huge dependency into our tree

FWIW this is the large dependency, browser-ui-test depends on puppeteer which installs two variants of Chrome postinstall. Worth mentioning that these Chrome distributions are non-free

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants