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

✨ feat: MetaMask setup in Cypress #1157

Merged
merged 12 commits into from
Jul 18, 2024
Merged

Conversation

matstyler
Copy link
Collaborator

@matstyler matstyler commented Jun 25, 2024

Motivation and context

Install MetaMask in Cypress.

Does it fix any issue?

https://linear.app/synpress/issue/SYN-124/setup-metamask-extension-for-cypress

Other useful info

Quality checklist

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough e2e tests.

⚠️👆 Delete any section you see irrelevant before submitting the pull request 👆⚠️

Copy link

vercel bot commented Jun 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
synpress ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 18, 2024 9:42am

@Seroxdesign
Copy link
Contributor

Seroxdesign commented Jun 26, 2024

Few things to note here:

Since we are doing some cacheless setup from what I can tell in your PR, and we're doing the same thing for Playwright in my current WIP PR. It might make sense to create a shared folder of functions for cypress/playwright.

Also have you run into any problems trying to run things using Cache in Cypress?

@Seroxdesign
Copy link
Contributor

You forgot to git ignore the download directory

@matstyler matstyler changed the title feat: MetaMask setup in Cypress part 1 feat: MetaMask setup in Cypress Jul 8, 2024
@matstyler matstyler marked this pull request as ready for review July 8, 2024 22:03
@matstyler matstyler requested review from Seroxdesign and drptbl July 8, 2024 22:03
@matstyler matstyler changed the title feat: MetaMask setup in Cypress ✨ feat: MetaMask setup in Cypress Jul 8, 2024
@Seroxdesign
Copy link
Contributor

Seroxdesign commented Jul 9, 2024

  1. Unit tests are failing
  2. Does this include using cache for Cypress? Here is a potential setup to make this happen
  3. I'm concerned we're going to have a difficult time with merging and dealing with conflicts given we've been developing in silos for the last month.

I've created a dev branch for v4 beta, I would appreciate if you could merge into there as we prep for V4 beta release, since we've been working on structural changes, leaving prs open for weeks, and dealing with conflicts can get very time consuming.

#1175

@matstyler
Copy link
Collaborator Author

  1. Unit tests are failing
  2. Does this include using cache for Cypress? Here is a potential setup to make this happen
  3. I'm concerned we're going to have a difficult time with merging and dealing with conflicts given we've been developing in silos for the last month.

I've created a dev branch for v4 beta, I would appreciate if you could merge into there as we prep for V4 beta release, since we've been working on structural changes, leaving prs open for weeks, and dealing with conflicts can get very time consuming.

#1175

  1. Yes, I pushed a few fixes today.
  2. No cache. It will be a separated task to cover that.

How is it better to use different branch? There will be the same conflicts here and there right?

@Seroxdesign
Copy link
Contributor

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

Copy link
Contributor

@Seroxdesign Seroxdesign left a comment

Choose a reason for hiding this comment

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

I am not sure if there's a way for us to shift to using better relative imports, the ../../ pattern is pretty ugly and confusing, but it's not critical

.gitignore Show resolved Hide resolved
wallets/metamask/src/cypress/MetaMask.ts Show resolved Hide resolved
wallets/metamask/src/prepareExtension.ts Outdated Show resolved Hide resolved
wallets/metamask/src/type/MetaMaskAbstract.ts Show resolved Hide resolved
wallets/metamask/test/cypress/metamask.cy.ts Show resolved Hide resolved
@matstyler
Copy link
Collaborator Author

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

But how the new branch will resolve the conflicts?

@Seroxdesign
Copy link
Contributor

Seroxdesign commented Jul 9, 2024

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

But how the new branch will resolve the conflicts?

I explained it ^

TLDR:

  1. faster merges because less review pressure
  2. because faster merges we can work off the latest changes

It's like having a dev branch, have you ever used a dev branch? Because you're not the only one pushing code now we need to find a way to keep in sync.

@matstyler
Copy link
Collaborator Author

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

But how the new branch will resolve the conflicts?

I explained it ^

TLDR:

  1. faster merges because less review pressure
  2. because faster merges we can work off the latest changes

It's like having a dev branch, have you ever used a dev branch? Because you're not the only one pushing code now we need to find a way to keep in sync.

But it's not helping with the conflicts. It's not a solution if we will have another, not synchronised branch. Lets try to make the way we have better aligned :)

@Seroxdesign
Copy link
Contributor

Seroxdesign commented Jul 9, 2024

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

But how the new branch will resolve the conflicts?

I explained it ^
TLDR:

  1. faster merges because less review pressure
  2. because faster merges we can work off the latest changes

It's like having a dev branch, have you ever used a dev branch? Because you're not the only one pushing code now we need to find a way to keep in sync.

But it's not helping with the conflicts. It's not a solution if we will have another, not synchronised branch. Lets try to make the way we have better aligned :)

You're being pedantic, dev branches are a well defined, useful standard that is popular and widely adopted for a reason.
The way you're working makes more work for me, and vice-versa.

For example, I already got things working on Windows, so if you're working off the latest branch you can just adopt the solution so I don't have to jump in and clean after you, or get my work blocked for another x time because windows works in 1 wallet and not another.

There are people using new-dawn 0.0.1-alpha.7, progress is made much slower there, which is why we are not in sync, having a dev branch like everyone else is a clean and simple solution. (:

@matstyler
Copy link
Collaborator Author

matstyler commented Jul 9, 2024

How is it better to use different branch? There will be the same conflicts here and there right?

The reason I am pushing for us using the new-dawn-dev branch is so that we don't have to hold off on merging our work together, for example:

  1. Right now we can't merge to new-dawn without thorough reviews, which is causing us to work in separate fragmented PRs, only dealing and updating PRs as ones make it through review
  2. When we make big structural changes we need to update/deploy our packages to npm, however some changes need to be done using workspace:* since our repo has many interdependencies and as far as I know only @drptbl can update npm
  3. it will allow us to work closer together and consider each-other's work, to hopefully reduce wasteful merge conflicts.
  4. It will allow us to organize the v4 BETA release under 1 branch and do a thorough test prior to launch.

I know it's a bit of a pace change but I'd really appreciate if we can adopt this.

But how the new branch will resolve the conflicts?

I explained it ^
TLDR:

  1. faster merges because less review pressure
  2. because faster merges we can work off the latest changes

It's like having a dev branch, have you ever used a dev branch? Because you're not the only one pushing code now we need to find a way to keep in sync.

But it's not helping with the conflicts. It's not a solution if we will have another, not synchronised branch. Lets try to make the way we have better aligned :)

You're being pedantic, dev branches are a well defined, useful standard that is popular and widely adopted for a reason. The way you're working makes more work for me, and vice-versa.

For example, I already got things working on Windows, so if you're working off the latest branch you can just adopt the solution so I don't have to jump in and clean after you, or get my work blocked for another x time because windows works in 1 wallet and not another.

There are people using new-dawn 0.0.1-alpha.7, progress is made much slower there, which is why we are not in sync, having a dev branch like everyone else is a clean and simple solution. (:

I am not saying about dev branch approach but about conflicts. You said you want to avoid them. I am just saying another branch will not resolve any conflict.

Furthermore how merging this branch somewhere else will help you will the conflicts in the future?

@matstyler matstyler requested a review from Seroxdesign July 9, 2024 13:10
.gitignore Outdated
@@ -37,6 +37,9 @@ test-results
playwright-report
playwright/.cache

### Cypress
wallets/metamask/downloads
Copy link
Collaborator

Choose a reason for hiding this comment

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

use wallets/**/downloads

supportFile: 'src/cypress/support/e2e.{js,jsx,ts,tsx}',
testIsolation: false,
async setupNodeEvents(on, config) {
return installSynpress(on, config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is installSynpress a good name here?
maybe configureSynpress would be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you think about "configureBeforeSynpress"?

wallets/metamask/package.json Show resolved Hide resolved
wallets/metamask/src/prepareExtension.ts Outdated Show resolved Hide resolved
"build:cache": "synpress-cache test/wallet-setup",
"build:cache:headless": "synpress-cache test/wallet-setup --headless",
"build:cache:headless:force": "synpress-cache test/wallet-setup --headless --force",
"build:cache": "synpress-cache test/playwright/wallet-setup",
Copy link
Collaborator

Choose a reason for hiding this comment

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

build:playwright:cache?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

end goal will be to use it for both: Playwright and Cypress

outputDir = ensureCacheDirExists()
} else {
outputDir = path.resolve('./', 'downloads')
outputDir = process.platform === 'win32' ? `file:\\\\\\${outputDir}` : outputDir
Copy link
Collaborator

Choose a reason for hiding this comment

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

why making it complicated, instead of doing

    outputDir = process.platform === 'win32' ? `file:\\\\\\${outputDir}` : path.resolve('./', 'downloads')

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are you sure it will work?

@@ -10,7 +10,7 @@ export default defineConfig({
fixturesFolder: 'src/cypress/fixtures',
testIsolation: false,
async setupNodeEvents(on, config) {
return installSynpress(on, config)
return configureBeforeSynpress(on, config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

configureBeforeSynpress?

why not
configureSynpress?

@matstyler matstyler requested a review from drptbl July 11, 2024 19:34
@drptbl drptbl merged commit 5544f21 into new-dawn Jul 18, 2024
10 checks passed
@drptbl drptbl deleted the feat/metamask-with-cypress branch July 18, 2024 19:53
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.

3 participants