E2e testing

[govza]

E2E tests for extension components

Priority*

• [ ] High: This PR needs to be merged first for other tasks.
• [x] Middle: This PR should be merged quickly to prevent conflicts due to common changes. (default)
• [ ] Low: This PR does not affect other tasks, so it can be merged later.

Purpose of the PR*

E2e testing is important and provides quick feedback if something goes wrong

Changes*

E2e package uses latest webdriverio which uses BiDi protocol with puppeteer for testing with browser different parts and behavior of the webextension example of github action result

How to check the feature

run pnpm build, pnpm zip, pnpm e2e or pnpm build:firefox, pnpm zip:firefox, pnpm e2e:firefox browsers will install webextension from generated zip file and run a number of tests.

There are tests for page content, content runtime, content ui, etc. Some tests are done providing workarounds for testing popup, devtools etc. components which are not available through BiDi protocol commands.

Reference

webdriverio webdriver-bidi puppeteer

[Jonghakseo]

I updated this branch to sync with main!

[govza]

I updated this branch to sync with main!

I usually do rebase / squash before merging anyway, I think it will be more straightforward in the git history.

[govza]

Names of the files inside /spec should be like others i mean e.g page-content and page-content-ui IMHO

It's a bit messy, many variants used, as for pages I see camel, kebab, and pascal: In pages: /pages/content/lib/toggleTheme.ts pages/content-ui/src/tailwind-input.css pages/options/src/Options.tsx

In packages: packages/hmr/lib/initializers/initClient.ts packages/hmr/lib/plugins/make-entry-point-plugin.ts

so IDK @Jonghakseo it's your call, if you want to formalize the naming conventions.

[Jonghakseo]

SOOOO GOOOD! But are there any additional settings or constraints need to run the test in a local environment besides the github action? When I ran it locally, I got an error message like below.

It works now on my local environment except i18n supports :) The new tab rendering test for the Toggle Theme button fails on my environment with i18n because it renders Korean text lol.

[PatrykKuniczak]

Names of the files inside /spec should be like others i mean e.g page-content and page-content-ui IMHO

It's a bit messy, many variants used, as for pages I see camel, kebab, and pascal: In pages: /pages/content/lib/toggleTheme.ts pages/content-ui/src/tailwind-input.css pages/options/src/Options.tsx

In packages: packages/hmr/lib/initializers/initClient.ts packages/hmr/lib/plugins/make-entry-point-plugin.ts

so IDK @Jonghakseo it's your call, if you want to formalize the naming conventions.

camel shouldn't be used for files or folder kebab - for file name pascal - for React Component

[govza]

@govza I was curious if there was a reason you removed zip from the e2e instruction dependency.

It looks overkill to regenerate zip from build each time before running e2e

[PatrykKuniczak]

@govza After you merge it, i will refactor names for entire project, to unify it

[govza]

It works now on my local environment except i18n supports :)

It uses local locales, maybe the issue. Probably the best way is to use other button selector.

I have been waiting for answers for the rest of my comments, and other thinks looks well.

Maybe I miss smth, some were addressed, some became outdated after changes. Please feel free to un-resolve if smth needs attention.

@govza After you merge it, i will refactor names for entire project, to unify it

Good idea! Maybe also add to readme about naming conventions for devs?

[PatrykKuniczak]

It works now on my local environment except i18n supports :)

It uses local locales, maybe the issue. Probably the best way is to use other button selector.

I have been waiting for answers for the rest of my comments, and other thinks looks well.

Maybe I miss smth, some were addressed, some became outdated after changes. Please feel free to un-resolve if smth needs attention.

@govza After you merge it, i will refactor names for entire project, to unify it

Good idea! Maybe also add to readme about naming conventions for devs?

Hymm, @Jonghakseo What do you think?

IDN if it's necessary, because if everywhere we have the same convention, they can see it easly

[govza]

That's not working at all

From what I see you need to build and zip it's not finding extension.zip file

Or export it into completely new tests next to packages and pages (This could be the best option, because users can create next subpackages with tests, e.g unit and etc. here)

Yeah, I was hesitant to do so, since unit tests are placed in their respective projects, but maybe there is use-case with api or some integration tests. Done.

[PatrykKuniczak]

@govza Yeah, i forgot about to run build and zip but i think this should be run before e2e, to avoid cases like that, and case when user don't have dev running and have old build and try to run tests.

If you don't want to rerun build on each test run, let's create e2e:watch

[PatrykKuniczak]

@govza I question, why during runtime soooo many warn ocurr?

There's no way to avoid it, and log only success and fail

This disturbs and looks messy

[govza]

If you don't want to rerun build on each test run, let's create e2e:watch

Build has to be run manually before zip, I'm not able to figure out proper turbo config, so I let someone else do it.

[PatrykKuniczak]

@govza Let's update package.json script e2e: pnpm build && pnpm zip && ...(e2e) that's it :)

And for watch, you need to add some flag for e2e script, almost every package which have CLI commands have --watch

[govza]

@govza Let's update package.json script e2e: pnpm build && pnpm zip && ...(e2e) that's it :)

And for watch, you need to add some flag for e2e script, almost every package which have CLI commands have --watch

I don't think it's correct to chain commands in package.json, it should be done in turbo, there's also complications with browsers env. I don't see watch command in this context, no running tasks here like hmr or smth.

[PatrykKuniczak]

@govza Then, let's do it with turbo, for now it's not good, to run commands chain by user for run e2e.

If there's not watch, let's try to find sth like nodemon for create a additional watcher for this, if it will be too tricky to implement, we can leave it like it is, because users can run dev and then e2e, but if tests want go on, then as you could know, we are trying to fix code and check tests again and again, that's because watcher will be very helpful.

In addition, we have watchers on almost everything, and each package was rebuilding after code change, it could be good to rerun test on code change like NestJS have.

But as i write above, if it will turn out very tricky for now, let's give up and try to do it in other PR.

For now, as i've typed above let's try to clean the console from this warns

[PatrykKuniczak]

@govza Let's update package.json script e2e: pnpm build && pnpm zip && ...(e2e) that's it :) And for watch, you need to add some flag for e2e script, almost every package which have CLI commands have --watch

I don't think it's correct to chain commands in package.json, it should be done in turbo, there's also complications with browsers env. I don't see watch command in this context, no running tasks here like hmr or smth.

I need to run this:

That's not good Dev Exp

[PatrykKuniczak]

@govza Now i see, what you're talking about.

To avoid this command chain let's use dependsOn key for turbo.json

PS: This solution isn't that good, because it doesn't support order, and commands are run in random order, this command chain ensure order are the same at each run

[PatrykKuniczak]

I accidentally found this, you can use it for enable watch for tests 😄 https://www.npmjs.com/package/npm-watch

It could be very helpful, to rerun tests on code change 😸

[govza]

Because this warn no longer ocurr :)

These are coming from webdriverio exception, probably bug, since tests are passing