-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix: [Placeholder] Message for simultaneous logins logout on web app #321
Conversation
Visit the preview URL for this PR (updated for commit df8d70e): https://ottwebapp--pr321-design-change-simult-06m3vmtc.web.app (expires Sun, 20 Aug 2023 09:29:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c198f8a3a199ba8747819f7f1e45cf602b777529 |
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.
lgtm
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 understand the learning curve with the tests sharing the same user, but I don't see a strong argument for changing it and in fact it results in adding boilerplate, making tests take longer, and reducing the coverage of actual flows. For the 4 subscription tests that all depend on one another I would be OK combining them provided you follow the suggestion for testing with logout and clearing the browser between steps.
We can go back and forth about the pros and cons of the sequential tests for days, but what I really want to understand is what were the actual bug fixes for the tests that were failing? Let's scale this PR down to just those and we can take the conversation for how we might make the test scenarios more developer friendly to another forum. |
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 have no other feedback :-)
Just a tip, I would use the Github Squash & Merge option to prevent cluttering the main branch with a lot of rework commits.
You can then also rewrite the commit message to refactor(auth): simultaneous login message from login component
to ensure that the changelog is (more) correct.
This has been fixed, and it is still in change request. |
This has been fixed, and it is still in change request.
Description
notion card: https://www.notion.so/jwplayer/Placeholder-Message-for-simultaneous-logins-logout-on-web-app-4328d1f4287d4291ab091e79bf49e126
This PR solves # .
Steps completed:
According to our definition of done, I have completed the following steps: