Open uchagani opened 3 months ago
Right now my workaround for retain-on-failure
option is to save the trace files via the @AfterEach
hook, and implement the TestWatcher
interface and delete the file if the test is successful.
@yury-s I'd like to start working on this next if that's agreeable with you. If so, can we use this issue to gather requirements? I had some ideas on how we could start tackling this in a phased approach.
The main open question related to tracing is how we want to manage per test artifacts. E.g in node.js version we have per test output directory where traces, screenshots etc are stored. The traced are copied there from temporary locations after the test has finished, probably we could follow the same pattern in java?
@yury-s if we allow the user to specify a trace directory, we can have a directory structure like this:
-traceDirFromOptions
-fully.qualified.test.class.dir
-test.name.zip
Does that work for you?
I'd try to follow as closely as possible the API/structure that we have in playwright test in node, because that way we'll be able to avoid some of the mistakes/regrets from the javascript version of playwright.
The structure seems fine but I suggest we use more generic outputDir
instead of traceDir
as it can be reused for screenshots etc. Some other considerations that come to mind:
beforeAll
, afterAll
, beforeEach
, afterEach
hooks and the test itself. There is fair amount of logic for that in the node.js version. We can probably refactor it upstream and expose over the internal protocol in driver similar to the methods in LocalUtils
to reuse the implementation. Not sure how feasible is that, but definitely worth trying.expect
asserts are included into the traces, would be nice to have standard JUnit asserts in the traces tooPLAYWRIGHT_JAVA_SRC
env variable.We need to find a way to merge together traces from beforeAll, afterAll,
Currently we don't provide BrowserContext
fixtures in @BeforeAll
or @AfterAll
hooks so do we have to worry about traces from there? If a user creates a new BrowserContext
in a class-level hook then they can manage the traces themselves for those contexts.
For @BeforeEach
or @AfterEach
hooks, as long as the tracing is done with the context provided by the fixture, everything should remain together. The part I am not sure how to handle is any calls to startChunk
or stopChunk
. I feel it's okay to let the user handle those as they are one offs and the fixtures only handle the the tracing recording that the fixtures start. What do you think?
In JavaScript expect asserts are included into the traces, would be nice to have standard JUnit asserts in the traces too
I don't think JUnit exposes any extension hooks into the assertions. I can think of two ways to do this:
Would be nice to automatically resolve source locations for the traces, so that it didn't require setting PLAYWRIGHT_JAVA_SRC env variable.
This will require some thought for sure. I don't have any ideas as of yet.
Would we be able to do this in a phased approach? I think allowing the automatic handling of traces for the contexts provided by the fixtures is important, especially being able to only save on failures as that is currently not possible without some ugly workarounds.
Currently we don't provide
BrowserContext
fixtures in@BeforeAll
or@AfterAll
hooks so do we have to worry about traces from there? If a user creates a newBrowserContext
in a class-level hook then they can manage the traces themselves for those contexts.
Good point. Let's let them manage it manually. before/aftewrAll
has been a source of confusion and more complex implementation in Node.js, so I'd rather we avoided it in Java.
For @BeforeEach or @AfterEach hooks, as long as the tracing is done with the context provided by the fixture, everything should remain together. The part I am not sure how to handle is any calls to startChunk or stopChunk. I feel it's okay to let the user handle those as they are one offs and the fixtures only handle the the tracing recording that the fixtures start. What do you think?
Yes, agreed. In Node.js currently startChunk
will throw if tracing is configured to be started by playwright test runner. I.e. users are allowed to either manually control traces or give up all control to the runner. I'd stick to the same approach here and not allow any startChunk/stopChunk/start/stop
calls if the tracing is configured with the fixtures. start*
methods will already throw if you call them on already started trace. For the rest it's probably sufficient to just document the behavior.
Another source of tracing complexity in Node.js is serial mode tests. This is when you can have all the tests in the same file reuse one and the same context. This is one of our regrets and if we were doing it today, serial mode wouldn't be there and we would be more adamant recommending alternatives like test.step()
. Luckily, we don't have serial mode in Java and don't need to implement all those shenanigans that we have in js.
One other feature that I believe we'll need to support sooner or later is managing traces for the tests that create more than 1 context. In Node.js version it is implemented via internal hooks on Browser.newContext()
, but this is because it was added as an afterthought. In Java we can expose a context factory fixture or some other mechanism for the clients to create additional contexts in a test with the same configuration parameters. I believe you already faced a similar problem that required making getOrCreate*
methods public. Let's think about making it more generic and public to all clients?
I don't think JUnit exposes any extension hooks into the assertions. I can think of two ways to do this
Yes, we'd need to hook into existing assertions one way or the other. I believe the tracing is useful even without those junit asserts, especially given all the web assertions that playwright provides. So we can think how to added it later.
Would we be able to do this in a phased approach?
I think so. Didn't mean to block this work on that.
🚀 Feature Request
This is also kind of a bug but I decided to make it a feature request.
Feature Request: I would like the ability to automatically save trace files.
Bug: I cannot save trace file based on test status.
Example
An option in
OptionsFactory
that allows the user to configure when to save trace filesoff
,on
,retain-on-failure
and an option for a root trace directory. The fixtures would then automatically start the trace, and depending on the option and the test result, would save the trace files in the root trace directory.Motivation
As I said, this is a feature request/bug report. I think the benefits are clear but the bug needs to be explained.
Currently, the user can only save a trace file by doing something like this:
There is no way for the user to only save trace files if the test fails (
retain-on-failure
). JUnit recommends implementing the TestWatcher interface. The problem is that the test watcher methods are called after theAfterEachCallback
methods, so when the test watcher method is called, theBrowserContext
is already closed by theBrowserContextExtension
. So even if I use reflection to get at theBrowserContext
inside myTestWatcher
, an exception is thrown.If we allow the fixtures to handle traces this issue can be easily solved