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

Add start of GUI tests for clippy lints page #13618

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

This is the beginning of adding GUI tests for the clippy lints page which had a number of regressions recently. I'll add (way) more if this is something the clippy team is interested into.

Now let me explain a bit why I picked this framework: browser-ui-test is meant to make it easy to write and read web GUI tests for people who are not familiar with web development (which is the case for most clippy contributors I suppose). It works by checking states of the various elements in the HTML page.

It also supports screenshots, however this feature is disabled by default because fonts have very small differences in differences in rendering, making screenshot comparisons mostly pointless unless they are all performed on only one computer, which in turn would limit contributors to add or update GUI tests. Even when not rendering fonts, the layout had some small subtle differences which again prevented the screenshots comparison to work.

This is the approach we decided to take for rustdoc and docs.rs and for now it seems to work nicely for both projects.

Don't hesitate if you have more questions.

changelog: Add GUI tests for clippy lints page

r? @flip1995

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 28, 2024
@GuillaumeGomez
Copy link
Member Author

@flip1995 and I talked about who would be best to review it and it seems only @Alexendoo have front-end development knowledge so assigning them as reviewer to this PR.

r? @Alexendoo

@rustbot rustbot assigned Alexendoo and unassigned flip1995 Oct 30, 2024
@bors
Copy link
Contributor

bors commented Nov 7, 2024

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

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts.

@Alexendoo: Don't hesitate if you need help for reviewing this PR.

@Alexendoo
Copy link
Member

I'm concerned it would be too much of a roadblock for contributors to the site. The tests rely on the page implementation details directly and updating these tests requires installing some pretty heavy dependencies (node/npm/puppeteer), plus learning a new DSL

I would be more comfortable with something like playwright tests since JS is at least familiar and there's auto complete to help out, but I still wouldn't be sure

@GuillaumeGomez
Copy link
Member Author

Well, testing websites will require something like puppeteer in any case. For the rest, seems like pretty good concern. I sadly don't have a better suggestion though as they all come with great cost.

I would have loved to have "something" when we were migrating the clippy lints page though.

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.

5 participants