microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
66.39k stars 3.63k forks source link

[Suggestion] Change HTML reporter configuration error message #28610

Closed edumserrano closed 10 months ago

edumserrano commented 10 months ago

When using Playwright's list reporter, if you configure the reporter's outputFolder to be somewhere inside the Playwright's outputDir then you get this error message:

image

See:

I wonder why is this being flagged as a Configuration Error ? At first when I saw this I thought the reporter wouldn't work since the message says there's an error but in the end the reporter still works as expected and does output the report in the expected folder.

I understand that the tests outputDir is cleaned in between test runs and that would mean I lose the data from the previous report but that seems fine to me. Isn't that even expected? New test run would equal new test results and therefore a new report right?

Maybe there's an edge case in here somewhere that I'm missing but at least I think perhaps the message shouldn't be flagged as an error but be a warning/suggestion? Especially considering that everything works as expected.

mxschmitt commented 10 months ago

We consider outputDir of the test-runner and outputDir of the html report two ecosystems which manage the control lifecycle itself. If you put one in another it would accidental remove one of another so this is a safety check we implemented in order to prevent that. (Since --list or the vscode extension would remove the test-results and by that the html report).

edumserrano commented 10 months ago

Thank you for the explanation. I can see why you guys implemented the check but isn't the check a worst case scenario check? Meaning the user can have one directory inside the other and never encounter any issues right?

I for one, haven't used the VSCode extension lately, I've mainly used the UI mode by itself. And in the way I work this configuration has never been a problem. The only issue is that I need to tell my team to ignore that Configuration error message. Or of course just change my configuration...

This is what I mean by worst case scenario check. It's a check for which the benefit will only be real depending on how people are using Playwright and the ecosystem of tools around it.

This is why I think moving it from an error to a warning would be more appropriate? It would be a beware of this because it might case you problems check.

Just my opinion of course, everything works as expected regardless.