-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add RecordingFileFactory with default implementation #507
Add RecordingFileFactory with default implementation #507
Conversation
By default this returns a file in the given vlc recording directory named '<RESULT>-<TestClassName>-<testMethodName>-<YYYYMMdd-HHmmss>.flv', where RESULT is either 'PASSED' or 'FAILED'. A custom factory can be provided to BrowserWebDriverContainer#withRecordingFileFactory. The factory is only applicable if the BrowserWebDriverContainer#recordingMode enables the retention of recordings. Fixes testcontainers#500.
|
||
@Override | ||
public File recordingFileForTest(File vncRecordingDirectory, Description description, boolean succeeded) { | ||
final String prefix = succeeded ? "PASSED" : "FAILED"; |
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.
I think we should put those Strings into constants.
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.
Sure.
@Override | ||
public File recordingFileForTest(File vncRecordingDirectory, Description description, boolean succeeded) { | ||
final String prefix = succeeded ? "PASSED" : "FAILED"; | ||
final String fileName = String.format("%s-%s-%s-%s.flv", |
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.
This String should also got into a constant.
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.
Sure.
File recordingFile = new File(vncRecordingDirectory, "recording-" + filenameDateFormat.format(new Date()) + ".flv"); | ||
|
||
private void stopAndRetainRecording(Description description, boolean succeeded) { | ||
File recordingFile = recordingFileFactory.recordingFileForTest(vncRecordingDirectory, description, succeeded); | ||
LOGGER.info("Screen recordings for test {} will be stored at: {}", description.getDisplayName(), recordingFile); |
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.
I know this is not part of your PR, but this line will fail for none JUnit usages (since no Description
will be available). Is the Description
actually needed?
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.
Ok. Is there a test-case for that in the codebase already?
Or better, if the description is null what should the factory default to?
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.
I see you are using Description
for the values of your recorder as well. I think it's okay for now, since we won't break the current API (which is kind of JUnit4 focused after all). We can refactor this part in the future. I'll check if it works like this with testcontainers-spock out of the box.
private void stopAndRetainRecording(Description description) { | ||
File recordingFile = new File(vncRecordingDirectory, "recording-" + filenameDateFormat.format(new Date()) + ".flv"); | ||
|
||
private void stopAndRetainRecording(Description description, boolean succeeded) { |
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.
Is there a way to rename this method such that the meaning of the parameter becomes more clear in the using code?
Maybe something along the lines of stopAndRetainRecordingForDescriptionAndSuccessState
(very long, yes =/).
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.
Sure. Need smalltalk / Objective-C / scala named args :-)
public static Collection<Object[]> data() { | ||
Random random = new Random(); | ||
Collection<Object[]> args = new ArrayList<>(); | ||
args.add(new Object[]{format("testMethodName%d", random.nextInt()), "FAILED", FALSE}); |
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.
Random values in unit tests? What do you need them for?
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.
Yep, overkill.
@@ -17,7 +18,8 @@ | |||
@Rule | |||
public BrowserWebDriverContainer chromeThatRecordsAllTests = new BrowserWebDriverContainer() | |||
.withDesiredCapabilities(DesiredCapabilities.chrome()) | |||
.withRecordingMode(RECORD_ALL, new File("./target/")); | |||
.withRecordingMode(RECORD_ALL, new File("./target/")) | |||
.withRecordingFileFactory(new DefaultRecordingFileFactory()); |
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.
Isn't this already used implicitly? Or did you want to make in explicit in this test?
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.
Yeah, it is used implicitly, but wanted to ensure that if you specifically attempted to provide your own factory that it would still succeed.
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.
Ok, makes sense.
I just restarted the CI build, failing was unrelated. |
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.
This is really good - thanks for the contribution. I just have a couple of comments on the code. Also, if you wouldn't mind adding at least a changelog comment it'd be good - this changes the output people will get, so we need to alert them. (Probably doesn't warrant a breaking change version bump though)
We will need to think about adding docs to describe this feature, but we can do that separately if you don't have time.
Thanks again for doing this!
prefix, | ||
getClass().getSimpleName(), | ||
methodName, | ||
DATE_FORMAT.format(new Date())) |
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.
I'm sorry to be pedantic, but I think there is a small risk that this test will be flaky... If it's running right on the transition from one second to the next, we're going to see a different date 😬
Would it be possible to do some assertions on the file name instead, e.g. a string comparison that ignores the time part?
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.
Sure. Freezing the date during the test would be ideal.
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.
Actually, how about I check for both now and one second ago?
|
||
import java.io.File; | ||
|
||
public interface RecordingFileFactory { |
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.
Would it be OK to change this to RecordingFileNameFactory
(and also rename the implementation)?
The reason I ask is that changing the filename seems to basically be the intent. The parent folder is already controlled separately, so all the user is really able to do is change the filename (unless I'm being very unimaginative 😄 )
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.
Certainly the original intent was to enable the file name to match the test name so that it was more easily identified.
Providing a custom factory, however, (if you want to alter the default we've now provided) means that setting the parent directory elsewhere is not necessary. i.e., you can ignore the vncRecordingDirectory argument.
I'm happy to rename it if you prefer?
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.
I wonder if the vncRecordingDirectory should be a property of the factory instead of the browser web driver container? That, of course, would be a breaking change—or perhaps a deprecated one.
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.
I've added docs now too.
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.
Let me know about the above thoughts. That, I think, is the last bit left.
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.
^^ I get it, OK. Let's keep as-is. I agree with the idea of setting the parent directory here, but as you say it would be a breaking change so let's save it for another day :)
Thank you also for the docs updates!
By default this returns a file in the given vlc recording directory named
<RESULT>-<TestClassName>-<testMethodName>-<YYYYMMdd-HHmmss>.flv
, where RESULT is eitherPASSED
orFAILED
.A custom factory can be provided to BrowserWebDriverContainer#withRecordingFileFactory.
The factory is only applicable if the BrowserWebDriverContainer#recordingMode enables the retention of recordings.
Fixes #500.