-
Notifications
You must be signed in to change notification settings - Fork 72
Add more authenticated end-to-end tests #880
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
Conversation
amyjko
left a comment
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.
This is a great new set of tests! And much needed.
The timeout is curious. Maybe it's imposed on PR workflows run by people not collaborators on a project? The docs say that timeouts should be 60 minutes.
11b9696 to
1ed7eb7
Compare
|
Hi @amyjko ! Made a couple more updates to get tests passing, and the PR is now ready for review!
Let me know if you'd like me to extract either of these changes into separate PRs! Also related to 2, I wonder if this is actually unintended behavior? It seems like the docs tile would still be helpful to see upon creating a new project. |
amyjko
left a comment
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.
This is create. The new Firebase interactions are clean and clear, the updates to the fixtures are more robust, and most importantly, we now have some critical basic tests for project and character saving. I particularly appreciate the reuse of the Firestore libraries in the API to ensure robust and precise confirmation of document writes, rather than a flaky reliance on interface updates. I'm going to approve as is.
Context
Add additional baseline authenticated tests using the new authentication flow introduced in f464e74. Also adds some test helpers to create a new (empty) project, create an new (empty) character, and interact with Firestore.
The new tests include (as discussed in #195 (comment)):
I also added a test to verify that character references in projects are updated when a character name changes, as this was not tested when I added the feature in #817, due to authenticated tests not being supported at the time. But now it is supported!
Related issues
Verification
Checklist
Verify tests pass in CIInvestigate test timeout issue (this seems to be an existing bug, recent PRs are also encountering test timeouts)