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

feature: Playwright e2e #77

Merged
merged 14 commits into from
Sep 10, 2024
Merged

feature: Playwright e2e #77

merged 14 commits into from
Sep 10, 2024

Conversation

gbarkhatov
Copy link
Contributor

@gbarkhatov gbarkhatov commented Aug 15, 2024

Adds basic Playwright example

https://youtu.be/r2zEOT_oVVo

Some basic tests:

  • Browser title
  • Wallet connection modal
  • Terms checkboxes
  • BBN wallet window injection
  • BBN wallet shown as browser wallet
  • Balance and address is correct after the connection
  • Preview is disabled until FP is selected and amount is filled
  • Staking TX is stored inside the local storage

Need your opinions on what do we improve in this PR and what do we extract to different issues

At the moment, having issues:

  • Properly using imports / exports. Cannot separate / extract BBN wallet, nor I cannot use BBN wallet as a mock Class. Object works though. Should be a way around, Playwright docs are good, and I see this as a future improvement.
  • Having issues importing external stuff. For example, mempool getBTCTipHeight
  • Number of tests are coming from my head, we can create a list of issues tracking what behavior we need to test. For example, we can be very specific on what our staking form behavior should look like.
  • I am not sure whether we want to keep e2e outside of tests to separate Jest and Playwright. There's also "integration" test in between these two - react-testing-library, that we don't currently use.
  • We need to think about the signPsbt in our mocked wallet. But it might be better handled in unbonding / withdrawing PR

Closes #110 and #13

@gbarkhatov gbarkhatov force-pushed the feature/playwright-e2e branch from 81379c8 to 0707a72 Compare September 2, 2024 11:47
@gbarkhatov gbarkhatov force-pushed the feature/playwright-e2e branch from 0707a72 to f361475 Compare September 2, 2024 20:40
@gbarkhatov gbarkhatov marked this pull request as ready for review September 2, 2024 20:47
e2e/app.spec.ts Outdated Show resolved Hide resolved
playwright.config.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
@jrwbabylonlab
Copy link
Collaborator

@gbarkhatov nice work! Feel more confident with future changes now! Just few comments and questions:

  1. Where are we mocking/passing the global param data? I assume we are making the actual API calls, right? It concerns me a bit on the relationship of mocked wallet response VS global params. Because the tests will be fragile to the global param changes. For example the activation height is 10 after X cap 2, and mocked wallet height is 5. then our tests will be broken due to this dependency.
  2. Mocked wallet. I think this is good enough approach for us since we are offloading the wallet part to the connector which itself shall be handling the e2e tests with different real wallets. So I think it's actually good to have mocks in the wallet for simple-staking to help us test the edge cases.
  3. I think you can leave it under the e2e directory. no opinion on this.
  4. "We need to think about the signPsbt in our mocked wallet. But it might be better handled in unbonding / withdrawing PR" For this, We can improve the mock to be more dynamic, i.e using libs to generate private keys and do proper signings etc. That's what we have done in the backend e2e tests. But of course, this is something we can improve later on.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

High-level comments and recommendations. Main comments revolve around having duplicated constants everywhere which will make the job of maintaining these tests very hard. Otherwise lgtm, approving pre-emptively as I don't have anything else to add and these can be handled in other PRs. However, I recommend that the constants issues are resolved in this PR.

e2e/helper/bbn_wallet.ts Outdated Show resolved Hide resolved
e2e/helper/bbn_wallet.ts Outdated Show resolved Hide resolved
e2e/helper/bbn_wallet.ts Outdated Show resolved Hide resolved
e2e/helper/bbn_wallet.ts Outdated Show resolved Hide resolved
e2e/helper/bbn_wallet.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
e2e/app.spec.ts Outdated Show resolved Hide resolved
@gbarkhatov
Copy link
Contributor Author

@jrwbabylonlab

Where are we mocking/passing the global param data? I assume we are making the actual API calls, right? Because the tests will be fragile to the global param changes

We need to think about the improvements there since E2E is actually checking out the system as a whole, so we should be careful with what we mock and when we use the real data.

This was referenced Sep 9, 2024
@gbarkhatov gbarkhatov merged commit 3f88fad into dev Sep 10, 2024
1 check passed
@gbarkhatov gbarkhatov deleted the feature/playwright-e2e branch September 10, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E tests for staking tx
3 participants