-
Notifications
You must be signed in to change notification settings - Fork 146
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
Support live server tests with Selenium #2077
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, I feel that there are some parts that could be cleaned up or need some more explanation otherwise:
d1316bf
to
a0bb516
Compare
Manager group was not present, but only if all tests are run. We found that django will first run Link dump if we try to debug this again:
We concluded that we want to migrate an empty database, dump it, and use that dump as a |
a0bb516
to
fd65ecd
Compare
cad398e
to
bb6a85c
Compare
@Kakadus @niklasmohrin @richardebeling Finally, the last test has been migrated and I'm ready for a final review. 🥳 |
6f8cb5d
to
88d3f6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet fully reviewed, will have to look through most of the ported tests still, but generally this is looking very nice. Thanks!
Getting chromium or firefox should be a lot easier now that we have nix, we can work something out together some time if you want :) |
d182ee9
to
a88cdfa
Compare
Heey, there is a green check now! 🎉 @hansegucker do you want to take over and resolve the open threads? If not, I can also finish this : ) Things I fixed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I especially like the changes to package.json and package-lock.json.
Haven't re-checked yet whether all ported tests assert the same things as the old tests, will need to re-do that in a final review.
if not self.tags and not self.exclude_tags: | ||
self.exclude_tags = {"live-server"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone uses the SkipLiveServerTestsRunner
and passes tags
or exclude_tags
here, it doesn't skip live server tests anymore. I'd either expect us to just add "live-server" to exclude_tags
, or at least assert that neither tags
or exclude_tags
was set (if we never expect this to happen). The current approach of continuing but with behavior that doesn't match the class name, to me, is just a trap we set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding --tags live-server
to start the live servers is reasonable. To mitigate the other stuff, I think this is most intuitive
if not self.tags and not self.exclude_tags: | |
self.exclude_tags = {"live-server"} | |
if "live-server" not in self.tags: | |
self.exclude_tags.add("live-server") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also a constant for the "live-server"
tag?
browser = "firefox" | ||
selenium_hub = None | ||
headless = True | ||
window_size = (1920, 3080) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3080 intended? Or should it be 1080?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the height should be large enough so that there is no scrolling needed. Maybe worth a comment?
66615d3
to
9e7c0f1
Compare
evap/staff/tests/test_live.py
Outdated
role=Contribution.Role.EDITOR, | ||
textanswer_visibility=Contribution.TextAnswerVisibility.GENERAL_TEXTANSWERS, | ||
role=Contribution.Role.CONTRIBUTOR, | ||
textanswer_visibility=Contribution.TextAnswerVisibility.OWN_TEXTANSWERS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this so that the values selected in the UI are not already selected. I found out this was intended to be a test for #1769 but I could not find there that this is required for this
I'm going to rebase this and sort the changes |
9e7c0f1
to
3db514f
Compare
3db514f
to
6c19036
Compare
Close #1854