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

Add ui tests for notifications #2019

Merged
merged 15 commits into from
Mar 22, 2022
Merged

Conversation

asnaith
Copy link
Member

@asnaith asnaith commented Mar 16, 2022

closes #2018

Adds tests for

  • card expiring soon notification
  • open crypto invoice notification

@asnaith asnaith added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Mar 16, 2022
@render
Copy link

render bot commented Mar 16, 2022

@render
Copy link

render bot commented Mar 16, 2022

@render
Copy link

render bot commented Mar 16, 2022

@asnaith asnaith marked this pull request as ready for review March 16, 2022 21:35
@asnaith asnaith marked this pull request as draft March 16, 2022 22:22
@asnaith
Copy link
Member Author

asnaith commented Mar 17, 2022

I realized that by introducing the test for the crypto invoice notification I have affected another test in the subscription plan spec related to crypto payments (it's not expecting an unpaid invoice row to be there at the beginning of the test).

I might have to see if there's a way to use separate sessions for each test to prevent a conflict as there's not a way to clear down that state afaik.

@Tbaut
Copy link
Collaborator

Tbaut commented Mar 17, 2022

I pushed commit to add a flag to generate a random session id, and force a new session.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just left a small comment

@@ -40,8 +40,9 @@ export const settingsPage = {
payNowButton: () => cy.get("[data-testid=button-pay-invoice]"),

// use this convenience function when an upgraded account is required as a test requisite
upgradeSubscription(plan: "pro" | "max") {
upgradeSubscription(plan: "pro" | "max", card?: "expiring") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a boolean here isCardExpired instead
If you want to make this easier to read, then instead of having 2 params, you can change this for object 1 param, with the type {plan: "pro"|"max", isCardExpired: boolean}. Then when you call this function, the params are easy to read, rather than func("pro", true), it'd be func({plan: "pro", isCardExpired: true})

edit: Maybe what you wanted to test is the fact that it's expiring this month, the name would change, but not the idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tbaut thanks for the explanation and letting me know about this approach, I've updated this now 🙏.

@Tbaut
Copy link
Collaborator

Tbaut commented Mar 17, 2022

It failed with the following, but it passed locally :(

|       1) can initiate and return to a crypto payment flow within 60 minutes
| 
| 
|   9 passing (3m)
|   1 failing
| 
|   1) Subscription Plan
|        desktop
|          can initiate and return to a crypto payment flow within 60 minutes:
|      CypressError: Timed out retrying after 4050ms: `cy.click()` failed because this element is `disabled`:

edit: This looks like an api issue. The returnToFreePlan flag fails.

@asnaith
Copy link
Member Author

asnaith commented Mar 18, 2022

edit: This looks like an api issue. The returnToFreePlan flag fails.

@Tbaut Unfortunately I got stuck on this and was unable to resolve it but I'm fairly confident it's not an API issue (well, it seems related to the API but not caused by the API). I've tried many things but am not fully understanding the underlying issue.

I can replicate the issue outside of CI though. Currently, with the two tests marked as it.only, the first time that you run them both they will pass as they both are using new sessions. Afterwards, if you then click the "run all tests" button to do a second run...

Screen Shot 2022-03-17 at 8 30 34 PM

The test that is restoring the session will fail on the resetToFreePlan every time, whilst the other test continues to run and pass as it is always using a new session. If you clear the sessions everything works again too (guess that's obvious 😄). So it seems like something weird is going on when using apiTestHelper after a session restore (maybe with the token refresh?).

Screen Shot 2022-03-17 at 8 36 26 PM

I'll continue to look at it tomorrow with a fresh brain but if you have any ideas please let me know (and thanks for your help so far with this!).

@Tbaut
Copy link
Collaborator

Tbaut commented Mar 18, 2022

Hey Andrew, from the suite of calls perspective, there's 100% something weird with the api. This should be tackled in this PR https://github.com/ChainSafe/files-api/pull/2082 I couldn't test with the api preview unfortunately so we'll have to wait until it's merged.

The problem is with the "return to free subscription" helper, that is failing for no apparent reason.

@Tbaut
Copy link
Collaborator

Tbaut commented Mar 21, 2022

ok I have some news. The "back to free plan" logic does not work if the user that we have is currently having a subscription awaiting payment, which is the case when we have a crypto test right before. There is no way we can go around this api limitation.

So the solution is to use my "newSesion" flag, every time we do something with crypto.

@asnaith asnaith marked this pull request as ready for review March 21, 2022 20:21
@asnaith
Copy link
Member Author

asnaith commented Mar 21, 2022

@Tbaut as mentioned on slack, the issues described above are now resolved by the latest api release on stage :)

@asnaith asnaith requested a review from Tbaut March 21, 2022 20:29
@asnaith asnaith merged commit 68e897a into dev Mar 22, 2022
@asnaith asnaith deleted the mnt/add-ui-tests-for-notifications branch March 22, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ui test coverage for notifications
3 participants