-
Notifications
You must be signed in to change notification settings - Fork 489
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: replace puppeteer with playwright for e2e tests #1802
Conversation
This seems odd:
It says it can't find the |
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.
@yusefnapora thanks for switching to playwright ! ❤️
On go-ipfs binary on CI
I wonder if the go-ipfs binary problem is related to ipfs/npm-kubo#36
Could be something else because the npm version did not change: same version was able to find go-ipfs binary just fine 2 days ago:
https://github.com/ipfs/ipfs-webui/runs/2787746156?check_suite_focus=true#step:3:11
We need to solve this before this is merged because E2E is part of CI of go-ipfs and js-ipfs CI setups, and we don't want to break them.
On js-ipfs
I see running E2E_IPFSD_TYPE=js npm run test:e2e
still fails, but now with playwright we should have more confidence when fixing it.
Give it a try, but f it turns out to be a rabbit hole we can merge this PR anyway and tackle js-ipfs at a later time in the future.
It seems like the go-ipfs issue was happening because I was using npm v7 locally, which generated a v2 lockfile. This apparently causes the postinstall script in go-ipfs to not run on ci. I re-generated the lockfile using npm v6, but then I started getting errors like this:
It seems there are some incompatibilities between Oddly, the jest issue doesn't show up using npm v7 - it's only broken if you install using npm v6. So there must be some significant change in dependency resolution logic between v6 and v7 that's causing it to break. Fun stuff. I just pinned jest to v26, which seems to fix the issue. It's not ideal to be stuck on v26 though, so it may be worth investigating further to see what's actually causing the problem. Or maybe the |
haha, okay, using lockfile v1 and jest v26 fixed the e2e tests (with go-ipfs anyway). But now the unit tests are failing with I found ipfs/js-ipfs#3620, but it seems odd to be getting this error with node v15, since |
Well, I squashed all the commits, then split out the dependency changes so I could revert At any rate, the go e2e tests and unit tests are back in business, so I'm going to quit while I'm ahead 😆 |
makes it clear what text was not found, and when search was aborted due to earlier error
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.
Thank you @yusefnapora this should be ok to merge.
(I did cosmetic tweaks to waitForText
in 387f8c6 just to make CI logs more intuitive)
We will tackle js-ipfs in #1737 and evaluate running against other vendors in #1804
I started pulling at this thread yesterday, since I hit some issues with puppeteer (possibly related to #1737).
This swaps out puppeteer for playwright. I haven't tested to see if that resolves the issue with js-ipfs in #1737, since I just got this working a few minutes ago and I should have switched gears a couple hours ago :) There's probably some stray console.logs and lint errors strewn about; I'll fix those up tomorrow.
cc @lidel closes #1164