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

Adds to gitignore a playwright.config.override.ts entry #49329

Merged
merged 2 commits into from
Apr 7, 2023

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented Mar 24, 2023

What?

When writing tests using the new Playwright suite of tools it's useful to set an overriding configuration where a developer can modify things like:

  • slowMo
  • when traces are created
  • if to run headless or not

Why?

To improve the speed of building tests with a better developer experience.

How?

Gitignore update.

Testing Instructions

  • Add a playwright.config.override.ts file in the /test/e2e/ folter.
  • Run git status you should not see the file.

@draganescu draganescu changed the title adds to gitignore playwright.config.override.ts Adds to gitignore a playwright.config.override.ts entry Mar 24, 2023
@draganescu draganescu added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. Developer Experience Ideas about improving block and theme developer experience labels Mar 24, 2023
@draganescu draganescu marked this pull request as ready for review March 24, 2023 09:56
Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

Pending @kevin940726 opinion, I think this is a good change

@github-actions
Copy link

github-actions bot commented Mar 24, 2023

Flaky tests detected in 72b32ae.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4510126260
📝 Reported issues:

@kevin940726
Copy link
Member

I'm not sure what the use case for this override config is though. You can already do much of the same thing by appending cli args.

slowMo

I find the trace viewer much more helpful than slowMo. I've never encountered a case where I have to enable that option. Note that slowMo slows down the tests which might hide some bugs which only happen when the interactions are fast.

However, if you really want to enable it in a test file temporarily, you can add this to the top of the file.

test.use( {
	launchOptions: {
		slowMo: 300,
	},
} );

when traces are created

Why would you ever want to change this? You can change this using CLI args though:

npm run test:e2e:playwright -- --trace on

if to run headless or not

You can also change this via CLI args:

npm run test:e2e:playwright -- --headed

I prefer keeping the interface of running the tests as simple as possible, so that we don't have to read yet another documentation than the official one. It also prevents the script from breaking when Playwright changes the config that's not compatible with the overriding algorithm. WDYT?

@draganescu
Copy link
Contributor Author

draganescu commented Mar 24, 2023

I understand your perspective and also shared your hesitation in bringing this in. I know we can just override everything using CLI args.

Nevertheless, I also think in general we should be able to have ways to override default configuration. Playwright is a tool and as a developer I should be able to define the way in which I want it to run. If I always want it to run in a certain way, it's a poor experience to always paste in some long line of args.

Also, every time we say "Why would you ever want to change this?" there is a very prescriptive tone underlying the question, as in "there is a way and you should always follow The Way". Playwright or WP Env are no Tao to me, they're tools. Who knows why, but I do like to run in headed slow mo while writing tests, maybe I can't visualize so well what it should happen after every line of the test. Maybe I am a noob 😊 in this way.

I also like the possibility to run temporary config updates in the actual test via use but, same as with simply making your own config, one needs to always be careful not to commit their noob ways of developing. There is also the problem with searching for help: almost anything you find about making playwright behave in a certain way will instruct one to alter their config.

Hence, I think, adding a gitignore rule for one file is a good change and given that the overriding does nothing special but use the same interface it should not be in danger of going stale.

P.S. UI mode is literally available since yesterday 😁 but it looks awesome. Still providing ways to configure your own tooling should not be an issue.

@kevin940726
Copy link
Member

Nevertheless, I also think in general we should be able to have ways to override default configuration. Playwright is a tool and as a developer I should be able to define the way in which I want it to run. If I always want it to run in a certain way, it's a poor experience to always paste in some long line of args.

We always have the option to provide our own config files via the --config arg, as shown in the doc in this PR. It's just that I don't think it should be encouraged because the default should "just work".

You can always create your own config files somewhere that git doesn't track and reference that when running the tests.

Also, every time we say "Why would you ever want to change this?" there is a very prescriptive tone underlying the question, as in "there is a way and you should always follow The Way". Playwright or WP Env are no Tao to me, they're tools. Who knows why, but I do like to run in headed slow mo while writing tests, maybe I can't visualize so well what it should happen after every line of the test.

Sorry about the tone, I certainly didn't mean that, I blame my English skill 😅. I agree that Playwright is just a tool, but in this case we're using this tool to run the e2e test suites for the gutenberg project. It becomes project-specific and should have a good default way to run the tests, so that we can ensure everyone gets consistent results.

There are some unanswered questions about providing your own custom config too. If we add something to the default config but the custom config isn't updated, then we might get different results. Even though we have the explicit --config arg, it could soon become a blind spot if the user is used to doing this.

Another potential risk is when merging the config. Sometimes we forget to extend a nested config and it could take ages to find the bug. Or sometimes it's just impossible to override a function config correctly.

In conclusion, I think we should aim for keeping the default config "just work", and document args for debugging if necessary. Overriding configs should be only for power users (like you!) who want to do more advanced stuff locally. I'm not against putting playwright.config.override.ts file to .gitignore, but I don't think we should write docs recommending this for regular users. But then again you can already do that now, but under a different path that git doesn't track (maybe ~/playwright.config.ts?).

I hope this all makes sense, and I'm not missing something obvious! Always thankful for engaging in such discussion! ❤️

@draganescu
Copy link
Contributor Author

Thanks for the clarifications @kevin940726 !

We always have the option to provide our own config files via the --config arg,

That doesn't work as we need to include things from the local codebase and playwright complains. For instance in a custom config we have to do import { defineConfig } from '@playwright/test'; and from another directory that does not work. Is there a way to do it? Expand @playwright to some absolute path?

It becomes project-specific and should have a good default way to run the tests

We do have a default way, this is about going around the default way. Running with a custom config is every developer's own risk.

I don't think we should write docs recommending this for regular users.

Sure, I can remove the docs!

If we can run the custom config from anywhere (like I was able to do in the past with puppeteer) this PR is useless. But can we?

And one last thing:

Note that slowMo slows down the tests which might hide some bugs which only happen when the interactions are fast.

I actually stumbled on a case for the navigation block where slowMo made the test fail correctly while it passed without it. Without slowMo I was able to select items before the menu was auto created :)

You are definitely not missing anything obvious and I understand the pushback. Maybe there is a way to just plop the file anywhere?

@kevin940726
Copy link
Member

For instance in a custom config we have to do import { defineConfig } from '@playwright/test'; and from another directory that does not work. Is there a way to do it?

I see! Yep, you are right! Now that I think about it, why don't we create a overrides folder at the root of the project and gitignore it instead? So that we don't have to keep updating .gitignore if we add another tool or config. WDYT?

@draganescu draganescu merged commit e9b4e69 into trunk Apr 7, 2023
@draganescu draganescu deleted the add/ignored-playwritght-override branch April 7, 2023 12:10
@github-actions github-actions bot added this to the Gutenberg 15.6 milestone Apr 7, 2023
@draganescu
Copy link
Contributor Author

why don't we create a overrides folder at the root of the project

I am all ok on that, but .. it seems too prescriptive? Maybe :) IDK. In general overrides are right there next to what they're overriding.

@kevin940726
Copy link
Member

Hi! Why is this merged? I thought we agreed to not include the docs?

@draganescu
Copy link
Contributor Author

Didn't I remove them 🤦🏻

@draganescu
Copy link
Contributor Author

@kevin940726 #49660 here to save the day 😄

@kevin940726
Copy link
Member

No worries, and thank you for the follow-up PR ❤️ .

@bph bph removed the [Type] Enhancement A suggestion for improvement. label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants