-
Notifications
You must be signed in to change notification settings - Fork 4
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
CU-8689rj9th Add Super Admin Tests #820
base: main
Are you sure you want to change the base?
Conversation
Task linked: CU-8689rj9th Enable super-admin tests on dev in CI |
Visit the preview URL for this PR (updated for commit 21f6766): https://roar-staging--pr820-add-super-admin-test-w5nizviu.web.app (expires Wed, 23 Oct 2024 20:40:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2631e9c58fd0104ecbfddd72a62245ddac467460 |
roar-dashboard-e2e Run #8488
Run Properties:
|
Project |
roar-dashboard-e2e
|
Run status |
Passed #8488
|
Run duration | 01m 09s |
Commit |
21f6766242: Super Admin Clean-Up Tests for PR 820 "CU-8689rj9th Add Super Admin Tests" from ...
|
Committer | Kyle |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
d5d9317
to
372d82d
Compare
2b53ebf
to
1a37b15
Compare
@ksmontville , can you rebase now that #821 is merged? |
7c0ce30
to
bb5cc87
Compare
ce53036
to
6206400
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.
I've only had a very quick look at the PR, more specifically the post-test cleanup logic and believe the current approach will cause test failures if implemented as-is.
From what I could see, and please correct me if I missed a part, the cleanup script is deleting any entry that matches these specified regexes – not just the ones that were created in the current test run.
In other words: if one PR runs the cleanup task whilst another PR is running tests, you'll be deleting data that is currently being used as part of separate test runs.
As your PR description says, this should ideally not be run against a remote environment but rather against the emulator, run in the CI. If we really have to get these tests merged before getting the emulators to work (?), then every test run needs to create database records that are identifiable. Whilst we cannot use the PR number for this as tests can be re-run, the workflow run_id
could do the job. Unfortunately, that approach means altering the application code to include this run ID, which is, well, not great.
Let me know what you think!
Thanks for the feedback. I have thought about this as well, and for now I think that we are okay because the tests which create the database entities (for example, creating a test adminstration) don't go back and check that they are in the database. They check for the "Success!" banner that pops up as a result of a successful database entity creation through Then the clean-up tests go through and delete any entities which match the regex, but they won't fail in the case where they don't find any particular entity. Ideally we would do this through emulators running |
68ad7c3
to
2815a0d
Compare
Proposed changes
Awaiting PR: #821 --> Merge this first to reduce files changed in this PR (there is some duplication).This PR adds support for running Cypress tests related to super admin functions on the dashboard. Currently, this includes creating test orgs, test administrations, test administrators, test students, and other general super admin user flows.
There are also new clean-up tests which will help keep Firestore from being inundated with test-related documents generated during these super admin tests.
Types of changes
What types of changes does this pull request introduce?
MAJOR CHANGES:
Cypress/support/query.js
andCypress/support/utils.js
Checklist
Justification of missing checklist items
Further comments