-
-
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 all 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 { | ||
File recordingFileForTest(File vncRecordingDirectory, Description description, boolean succeeded); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
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.time.format.DateTimeFormatter; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Collection; | ||
import java.util.List; | ||
|
||
import static java.lang.Boolean.FALSE; | ||
import static java.lang.Boolean.TRUE; | ||
import static java.lang.String.format; | ||
import static java.time.LocalDateTime.now; | ||
import static org.hamcrest.core.IsCollectionContaining.hasItem; | ||
import static org.junit.Assert.assertThat; | ||
import static org.junit.runner.Description.createTestDescription; | ||
|
||
@RunWith(Parameterized.class) | ||
@Value | ||
public class DefaultRecordingFileFactoryTest { | ||
|
||
private static final DateTimeFormatter DATETIME_FORMATTER = DateTimeFormatter.ofPattern("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); | ||
|
||
String expectedFilePrefix = format("%s-%s-%s", prefix, getClass().getSimpleName(), methodName); | ||
|
||
List<File> expectedPossibleFileNames = Arrays.asList( | ||
new File(vncRecordingDirectory, format("%s-%s.flv", expectedFilePrefix, now().format(DATETIME_FORMATTER))), | ||
new File(vncRecordingDirectory, format("%s-%s.flv", expectedFilePrefix, now().minusSeconds(1L).format(DATETIME_FORMATTER))) | ||
); | ||
|
||
assertThat(expectedPossibleFileNames, hasItem(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.
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!