-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from 3 commits
b08541e
30dcbb7
0798524
4462951
b42082f
bb28bed
e014048
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package org.testcontainers.containers; | ||
|
||
import org.junit.runner.Description; | ||
|
||
import java.io.File; | ||
import java.text.SimpleDateFormat; | ||
import java.util.Date; | ||
|
||
public class DefaultRecordingFileFactory implements RecordingFileFactory { | ||
|
||
private static final SimpleDateFormat filenameDateFormat = new SimpleDateFormat("YYYYMMdd-HHmmss"); | ||
private static final String PASSED = "PASSED"; | ||
private static final String FAILED = "FAILED"; | ||
private static final String FILENAME_FORMAT = "%s-%s-%s-%s.flv"; | ||
|
||
@Override | ||
public File recordingFileForTest(File vncRecordingDirectory, Description description, boolean succeeded) { | ||
final String prefix = succeeded ? PASSED : FAILED; | ||
final String fileName = String.format(FILENAME_FORMAT, | ||
prefix, | ||
description.getTestClass().getSimpleName(), | ||
description.getMethodName(), | ||
filenameDateFormat.format(new Date()) | ||
); | ||
return new File(vncRecordingDirectory, fileName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package org.testcontainers.containers; | ||
|
||
import org.junit.runner.Description; | ||
|
||
import java.io.File; | ||
|
||
public interface RecordingFileFactory { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be OK to change this to 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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! |
||
File recordingFileForTest(File vncRecordingDirectory, Description description, boolean succeeded); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
package org.testcontainers.containers; | ||
|
||
import lombok.Value; | ||
import org.junit.Test; | ||
import org.junit.runner.Description; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.Parameterized; | ||
|
||
import java.io.File; | ||
import java.nio.file.Files; | ||
import java.text.SimpleDateFormat; | ||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Date; | ||
|
||
import static java.lang.Boolean.FALSE; | ||
import static java.lang.Boolean.TRUE; | ||
import static java.lang.String.format; | ||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.runner.Description.createTestDescription; | ||
|
||
@RunWith(Parameterized.class) | ||
@Value | ||
public class DefaultRecordingFileFactoryTest { | ||
|
||
private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("YYYYMMdd-HHmmss"); | ||
|
||
private final DefaultRecordingFileFactory factory = new DefaultRecordingFileFactory(); | ||
private final String methodName; | ||
private final String prefix; | ||
private final boolean success; | ||
|
||
@Parameterized.Parameters | ||
public static Collection<Object[]> data() { | ||
Collection<Object[]> args = new ArrayList<>(); | ||
args.add(new Object[]{"testMethod1", "FAILED", FALSE}); | ||
args.add(new Object[]{"testMethod2", "PASSED", TRUE}); | ||
return args; | ||
} | ||
|
||
@Test | ||
public void recordingFileThatShouldDescribeTheTestResultAtThePresentTime() throws Exception { | ||
File vncRecordingDirectory = Files.createTempDirectory("recording").toFile(); | ||
Description description = createTestDescription(getClass().getCanonicalName(), methodName, Test.class); | ||
|
||
File recordingFile = factory.recordingFileForTest(vncRecordingDirectory, description, success); | ||
|
||
File expectedFile = new File(vncRecordingDirectory, format("%s-%s-%s-%s.flv", | ||
prefix, | ||
getClass().getSimpleName(), | ||
methodName, | ||
DATE_FORMAT.format(new Date())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Actually, how about I check for both now and one second ago? |
||
); | ||
assertEquals(expectedFile, recordingFile); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import org.junit.Test; | ||
import org.openqa.selenium.remote.DesiredCapabilities; | ||
import org.testcontainers.containers.BrowserWebDriverContainer; | ||
import org.testcontainers.containers.DefaultRecordingFileFactory; | ||
|
||
import java.io.File; | ||
|
||
|
@@ -17,7 +18,8 @@ public class ChromeRecordingWebDriverContainerTest extends BaseWebDriverContaine | |
@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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ok, makes sense. |
||
|
||
@Rule | ||
public BrowserWebDriverContainer chromeThatRecordsFailingTests = new BrowserWebDriverContainer() | ||
|
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 theDescription
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.