-
Notifications
You must be signed in to change notification settings - Fork 190
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(junit): Implement automatic saving of traces and screenshots via fixtures #1560
base: main
Are you sure you want to change the base?
Conversation
I don't think the failures here are related to the PR. could we try re-running? |
Restarted. But it looks like at least some of them are in the new tests. |
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.
Looks good, but let's figure out the multithreading issues and other failures of the new tests on the bots.
playwright/src/main/java/com/microsoft/playwright/junit/Options.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/junit/BrowserContextExtension.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/junit/BrowserContextExtension.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/junit/BrowserContextExtension.java
Show resolved
Hide resolved
playwright/src/test/java/com/microsoft/playwright/junit/TestTracesAndScreenshots.java
Outdated
Show resolved
Hide resolved
playwright/src/test/java/com/microsoft/playwright/junit/TestScreenshotOn.java
Outdated
Show resolved
Hide resolved
playwright/src/test/java/com/microsoft/playwright/junit/Transient.java
Outdated
Show resolved
Hide resolved
playwright/src/test/java/com/microsoft/playwright/junit/TestTracesAndScreenshots.java
Show resolved
Hide resolved
playwright/src/test/java/com/microsoft/playwright/junit/TestTracesAndScreenshots.java
Outdated
Show resolved
Hide resolved
@yury-s the threading issues, I believe, are resulting from tests running via the engine kit and the normal way at the same time. I believe the threading issues are due to use using threadlocal in our extensions. Somewhere, playwright is getting closed by the ExtensionContext but there are still tests running on that thread. We should eventually move away from using threadlocal and store objects in the ExtensionContext (this is what Junit recommends). However, I'm not sure how we can scope to threads while doing that. We can scope to method or class easily but we want to reuse playwright and keep it open across test classes. I don't think users will be affected by this but will verify by running the whole suite except engine kit tests. |
@yury-s i believe i have addressed all the comments. I also verified that the threading issues are not user-facing and should not affect users. After this PR, I can start looking into how we can store playwright fixtures in the ExtensionContext and scope them to specific threads |
will work on test failures this evening |
I'm sorry for the delay life got in the way recently. Hopefully will get to this this weekend |
No problem at all, hope everything is alright. |
Resolves #1526. This PR also adds saving screenshots.
I tried to keep the API of the fixture-generated artifacts as close to node as possible.
Traces:
By default, if no
outputDir
is specified in theOptions
, the default output path will beprojectDir/test-results/fully.qualified.ClassName.testMethodName-browserName/trace.zip
. If anoutputDir
is specified in theOptions
then the artifacts will go inuserSpecifiedOutputDir/fully.qualified.ClassName.testMethodName-browserName/trace.zip
.Screenshots:
Screenshots follow the same nomenclature as the traces with the exception that screenshot files are named
test-finished-1.png
. Depending on the number of pages aBrowserContext
has, the1
will be incremented perPage
.I added tests to test this feature. Since traces and screenshots are created after the test is finished, I needed to add a new test dependency (
junit-platform-testkit
). This is JUnit's preferred way to test extensions.@Transient
annotation was created because the new tests should not be run by themselves and only run through theEngineTestKit
. I updated the GitHub actions to exclude these tests.There is some weird threading issues happening when the new tests run with the existing suite so I added a new step in the CI actions to run the new tests separately.