Organization and improvements of test runners

[bedrich-schindler]

Based on #588, I am opening discussion about organization and improvements of test runners.

After #588 is merged, we will be in situation when we have npm test command that just runs npm run test:jest, but not npm run test:playwright:all. You need to run npm run test:playwright:all separately in Playwright Docker container to have uniform testing environment to generate correct snapshots.

What more, if running tests on Apple Silicon, you always need to run npm ci within Docker (in Node shell or Playwright, it does not matter) to ensure compilation of rollup/vite for correct architecture.

My suggestions:

1. I would merge Node shell and Playwright and you only one Node container with preinstalled browsers.
2. I would require running the project in the Docker. It means that (optional) statements would disappear from the documentation.
3. I would allow to run npm run test:playwright:all with npm test
4. I would add pre-scripts to npm ci, npm test (and all Playwright related scripts) to ensure that it is run inside Docker only, otherwise it fail with error.

This would result in uniform environment, uniform approach that would be controlled (and would failed if running on host machine), less requimenets for disk space and more straightforward organization of npm scripts.

I would also suggest prefixing all linters with lint:, so that all linters are grouped, all test runners are grouped.

cc @adamkudrna @mbohal @atmelmicro @lukasbriza

[adamkudrna]

Ad 1: I'm not sure. If I'm not wrong, the versions of the Playwright npm package and Docker image must match. That means any update to the npm package would require rebuilding the development Docker container. Are we ready for that?

Ad 2: According to CONTRIBUTING.md, Docker is already mandatory. I use it like that.

Ad 3: Before getting to this proposal, I commented on the PR #588 mentioned here. I don't think running visual tests along with other tests, i.e. during npm test, is practical. Visual tests are recource-heavy and time-consuming and there are cases when you really don't need them (see my comment ⬆). That's why I prefer them a) not to be part of npm test, b) to be opt-in for PRs, and (because of that) c) to be run regularly on the master branch.

Ad 4: Since 2) is already in place, let's do it, why not.

[bedrich-schindler]

1. This is true, but we can make it other way. playwright docker image uses ubuntu image by default and always installs Node. So while playwright is shipped with Node, we can use playwright instead of node_shell as well. It is true that depending on node docker image is straighforward and fool-proof, but it is possible way in case we do not want to build image every time. We just need to keep track of Node version installed inside playwright docker image. See source.
2. You are right. Docs was improved in Playwright component testing #588.
3. Let's open the discussion about this on Monday. I just expect that npm test is there to test everything (this is what Tomas Litera said on meetup as well) and current state is not pleasant. But you are totally right that it can be time consuming, but ... We have to agree on "some approach that is better than current state". This can include running Playwright tests only when necessary - but it is hard to determine when to run them as even the maintenance task can produce change in visual snapshots as well. We are only safe if we edit just MD files. We were also testing all components with Jest on every push and this is the same. I would like to point out that there is difference between Playwright stack you use in Alma and our stack as we will get rid of Jest for most of cases speaking of components. So without running Playwright tests every time, we say we just skip tests for all components and since Playwright tests will be only tests (which is not same as in Alma), no test can be run. So discuss in personally as it can be easier.

New things that raised:

5. We are in the strong diasgreement about naming - Playwright tests vs Visual tests. I think that it kinda depends on the problem number 3. Again, I am not confident that Playwright tests or Playwright is good naming, but at least during testing phase it a) strongly distinguishes old and new (Playwright) stack; b) it is correct name in the meaning that it covers not only visual testing but also functional testing so calling in visual tests or e2e tests in not correct either. I think this should be discussed together with problem number 3.
6. How to run Playwright in Github CI? Approach documented by Playwright is just to install browsers and run Playwright directly from Github CI. @adamkudrna recommended approach from Spirit where they run it inside of Docker. Problem is hard coded version in workflows. However, this is connected to point 1.

[lukasbriza]

I agree with the points mentioned in the first comment #590 (comment) . Regarding Playwright tests, I support running them every time, as we are primarily working with a component library where the visual aspect is crucial. Since Playwright tests will be the main testing mechanism for components, I see no reason to skip them.

I also think that merging the Docker containers for node_shell and Playwright is a good idea. It would simplify the setup and reduce overhead while still keeping the necessary dependencies in place.

[adamkudrna]

playwright docker image uses ubuntu image by default and always installs Node.

OK, I still find it a little bit unstraightforward, but I see what you mean. Let's try it!

3. I would like to point out that there is difference between Playwright stack you use in Alma and our stack as we will get rid of Jest for most of cases speaking of components. So without running Playwright tests every time, we say we just skip tests for all components and since Playwright tests will be only tests (which is not same as in Alma), no test can be run.

I think this is the very key point of the whole debate. I did not realize we are dropping Jest and after #588, Playwright is not the main testing mechanism, it's the only testing mechanism. I still expect the tests to be time consuming though, so it might make sense to skip them for 100%-safe edits like MD files (yes, there is no doubt that almost any change in the repository can break visual tests something).

Approach documented by Playwright is just to install browsers and run Playwright directly from Github CI.

Apparently, things may have changed since the time we implemented Playwright in Spirit. Let's do it as recommended.

[bedrich-schindler]

@mbohal, please read through this and provide your statement. We can discuss this next Monday.

[mbohal]

I would merge Node shell and Playwright and you only one Node container with preinstalled browsers.

I'm afraid, that we might need/want to move to a newer version of Node.js sooner then the Playwright Docker image. I see no relationship between Node.js in Playwright and Node,js we use for our stack. We could even run playwright tests with python (https://mcr.microsoft.com/en-us/artifact/mar/playwright/python/about) and have no Node.js available at all.

I think about Node.js being available in the Node.js image as a convenient accident and I would prefer not to depend on it for development.

I recognize it would make life easier, but I don't think it will be worth it in the long run.

Since Playwright tests will be the main testing mechanism for components, I see no reason to skip them.

I agree with @lukasbriza, unless:

it might make sense to skip them for 100%-safe edits like MD files

This would be the best I think. Detect if all changes happened in *.md files only and if so, skip Playwright tests. I'm not sure though for how many PRs this true. It makes no sense to spend hours configuring this if it is going to speed up 3 PRs per year.

[bedrich-schindler]

Few weak ago on team meeting, we decided not to merge Playwright and Node images as we do not want to relate on Microsoft's decision on which version to use. We are keeping those separated.

By this, all points have been discussed with no output, so I am closing this.