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

Refs #34043 -- Added GitHub action to capture screenshots in Selenium tests. #16963

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

sarahboyce
Copy link
Contributor

@sarahboyce sarahboyce commented Jun 9, 2023

This is a proposal to help us confidently make UI changes. 💪
Testing UI is always hard and many of us would not describe ourselves as frontend developers. Adding a layer of visual testing to our automated test suite will not only make our lives easier but can open up possibilities to us such as tests to cover visual regressions.

In ticket 34043, there is a suggestion that we should have a test suite/tooling to keep track of UI changes automatically.

This PR adds the screenshot tests to our GitHub actions. This is currently triggered when a PR has the selenium label.
You can see the screenshots (in a zip file at the bottom called screenshots) here: https://github.com/django/django/actions/runs/5386413722?pr=16963

The action "fails" because I do not have write permissions to the repo. please see this comment #16963 (comment). I have tested this against my fork where I do have write permissions. This should be resolved on merge.

@sarahboyce sarahboyce force-pushed the ci-test branch 3 times, most recently from d4d6814 to 5ef7a92 Compare June 9, 2023 19:42
@smithdc1
Copy link
Member

smithdc1 commented Jun 9, 2023

I'm not sure if this is helpful? Maybe you've already seen it!

https://simonwillison.net/2022/Mar/10/shot-scraper/

@sarahboyce
Copy link
Contributor Author

sarahboyce commented Jun 9, 2023

I'm not sure if this is helpful? Maybe you've already seen it!

https://simonwillison.net/2022/Mar/10/shot-scraper/

I haven't but it's quite similar (I am also using playwright here but using the python version because Python = ❤️), will take a look!
I am trying also take inspiration from Adam's blog (https://adamj.eu/tech/2023/03/17/django-parameterized-tests-model-admin-classes/) around testing the admin and what I want in the end is something that will give you just changed screenshots with before and after so hopefully the css changes are easier when seeing unexpected changes (visual regressions are just difficult)

As an example, you can download and see screenshots here in the Artifacts section at the bottom: https://github.com/django/django/actions/runs/5225589580, it's not quite what I want yet but you get the idea. Need to check with people if we think this adds value.

Crossing out comment as the approach has since changed, leaving this comment misleading.

@sarahboyce sarahboyce force-pushed the ci-test branch 11 times, most recently from 0b9ae54 to c119757 Compare June 10, 2023 18:04
@sarahboyce sarahboyce changed the title Refs #34043 -- Add a github action to help test admin ui. Refs #34043 -- Added a GitHub action for testing ui changes. Jun 10, 2023
@sarahboyce sarahboyce force-pushed the ci-test branch 11 times, most recently from 184859e to fa1cc06 Compare June 11, 2023 14:02
@sarahboyce sarahboyce force-pushed the ci-test branch 2 times, most recently from 67bfeb0 to 7478d47 Compare October 12, 2023 21:26
@felixxm
Copy link
Member

felixxm commented Oct 16, 2023

@sarahboyce Thanks for all your efforts 👍 I would like to merge/discuss the remaining code in two steps:

  1. new screenshots option to the runtests.py,
  2. new GitHub action.

@sarahboyce sarahboyce force-pushed the ci-test branch 2 times, most recently from 787e83d to 280fda2 Compare October 16, 2023 10:06
django/test/selenium.py Outdated Show resolved Hide resolved
@sarahboyce sarahboyce force-pushed the ci-test branch 2 times, most recently from 5cf7db3 to fa5197e Compare October 17, 2023 10:26
@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

I moved the first commit with small edits (release notes, comments) to the #17374.

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

I moved the first commit with small edits (release notes, comments) to the #17374.

Merged in be56c98.

@felixxm felixxm changed the title Fixed #34043 -- Added screenshots to admin tests and clarified contributing to UI. Refs #34043 -- Added GitHub action to capture screenshots in Selenium tests. Oct 18, 2023
@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

@sarahboyce Many thanks! rebased.

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

I simplified the generated comment a bit to:

image

@ngnpope
Copy link
Member

ngnpope commented Oct 18, 2023

I simplified the generated comment a bit to:

image

This reverts a bunch of what was discussed in this comment, in particular reducing the visibility of screenshots taken on previous runs. Being able to easily access multiple downloads is useful for comparison.

Maybe an alternative is not to edit existing comments, but just create new ones every time?

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

Maybe an alternative is not to edit existing comments, but just create new ones every time?

Yes, I was thinking about the same 👍

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

@ngnpope You can check the result in felixxm#6.

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

This also make it easier to compare results when pushing fixes.

@sarahboyce
Copy link
Contributor Author

sarahboyce commented Oct 18, 2023

Maybe an alternative is not to edit existing comments, but just create new ones every time?

Yes, I was thinking about the same 👍

I'm not a fan of a new message with each push/commit. When you get enough comments, like this PR, conversations become hidden even when they're not resolved. I missed that Nick's suggestion earlier was to append to an existing message. Can try that? Or that a message is created in a thread? Worried a new message each push/commit might pollute a review process 🤔

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

Let's check how it works with updating a comment, felixxm#8.

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

All good, but date is not there 🤔

@felixxm
Copy link
Member

felixxm commented Oct 18, 2023

Finally, it works! 🎉 felixxm#9

@sarahboyce
Copy link
Contributor Author

Finally, it works! 🎉 felixxm#9

Ah I love it!

@felixxm felixxm merged commit 97b7970 into django:main Oct 18, 2023
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
screenshots 🖼️ selenium Apply to have Selenium tests run on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants