Skip to content
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

Spike API for attaching files to test executions #3336

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

marcphilipp
Copy link
Member

No description provided.

@marcphilipp marcphilipp added this to the 5.11 M1 milestone May 30, 2023
Comment on lines 49 to 61
@Test
void reportFiles(TestReporter testReporter, @TempDir Path tempDir) throws Exception {

testReporter.publishFile("test1.txt", file -> Files.write(file, singletonList("Test 1")));

Path existingFile = tempDir.resolve("test2.txt");
testReporter.publishFile(Files.write(existingFile, singletonList("Test 2")));

testReporter.publishFile("test3", dir -> {
Path nestedFile = Files.createDirectory(dir).resolve("nested.txt");
Files.write(nestedFile, singletonList("Nested content"));
});
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This demonstrates what the programming model for test written with JUnit Jupiter would look like.

*/
@API(status = EXPERIMENTAL, since = "5.11")
default void publishFile(String fileName, ThrowingConsumer<Path> action) {
throw new UnsupportedOperationException();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface shouldn't be implemented by clients but is annotated with @FunctionalInterface so adding another abstract method would, at least theoretically, break backward compatibility. We could decide to ignore that in this case since nobody should have implemented this, go with this pattern, or introduce a separate FileReporter interface that can be injected alongside TestReporter or could extend it and replace it.

Comment on lines +252 to +256
eventsFileWriter.append(reported(id, Instant.now()), //
reported -> reported.append(attachments(), attachments -> attachments.append(file(), file -> {
file.withTime(entry.getTimestamp());
file.withPath(outputDir.toPath().relativize(entry.getFile()).toString());
})));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reported like this in the XML file:

<e:reported id="25" time="2023-05-30T19:00:31.623824Z">
    <attachments>
        <junit:file
                path="junit-5012861544611183477/junit-jupiter/example.TestReporterDemo/reportFile(org.junit.jupiter.api.TestReporter, java.nio.file.Path)/test1.txt"
                time="2023-05-30T21:00:31.621204"/>
    </attachments>
</e:reported>

Instead of the junit namespace, this should be declared in the core namespace of open-test-reporting. I only did it here to avoid having to work in two repos for this spike.

ExecutionRequest request = new ExecutionRequest(engineTestDescriptor, listener,
discoveryRequest.getConfigurationParameters());
ExecutionRequest request = ExecutionRequest.create(engineTestDescriptor, listener,
discoveryRequest.getConfigurationParameters(), OutputDirProvider.NOOP);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing with EngineTestKit a special implementation of OutputDirProvider should be provided that uses a temporary directory that is cleaned up after the surrounding test is finished.

Copy link
Member

@dsaff dsaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely feels like it's a good direction! My primary design question is whether we'd want a way to attach "types" (MIME types, or something else?) to these files.

In a separate thread of thought, how could we make this available for people still using JUnit 4?

* @see #from(Path)
*/
@API(status = EXPERIMENTAL, since = "1.11")
public final class FileEntry {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debating whether we'd want some kind of MIME type, or if we'd trust report users to use extensions or content analysis to know which files were, for example, displayable as images...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a case for an optional mime type but making it mandatory is probably too heavy a burden on users. I'd expect one would browse the output dir in a file explorer or web browser at some point which will rely mostly on file extensions anyway. We could add a mime type later or right away. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it for later seems fine.

@marcphilipp
Copy link
Member Author

This definitely feels like it's a good direction! My primary design question is whether we'd want a way to attach "types" (MIME types, or something else?) to these files.

see #3336 (comment)

In a separate thread of thought, how could we make this available for people still using JUnit 4?

I was wondering that as well. I'm hesitant to change JUnit 4's RunListener API to add a method that can publish a file. However, we could provide a TestRule implementation that has an API similar to the methods added in this PR to TestReporter. The rule would only work when used via the Vintage engine. Otherwise publishing files would be a no-op. The Vintage engine could provide a backing reporter mechanism via a ThreadLocal. If you think that might be viable, I can take a stab at spiking it.

@marcphilipp marcphilipp requested a review from dsaff July 29, 2023 10:06
@marcphilipp
Copy link
Member Author

@leonard84 How would you expose this to Spock tests?

@marcphilipp
Copy link
Member Author

The Vintage engine could provide a backing reporter mechanism via a ThreadLocal.

On second thought, maybe a ServiceLoader-based approach would be better.

@marcphilipp
Copy link
Member Author

@dsaff I pushed a POC for how it could work for JUnit 4

Comment on lines +23 to +36
public class VintageTestReportingDemo {

@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();

@Rule
public TestReporting testReporting = new TestReporting();

@Test
public void reportFiles() throws Exception {
Path existingFile = Files.write(temporaryFolder.getRoot().toPath().resolve("test.txt"), singletonList("Test"));
testReporting.publishFile(existingFile);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JUnit 4 example

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really clean!

@marcphilipp marcphilipp modified the milestones: 5.11 M1, 5.12 Jan 12, 2024
@marcphilipp marcphilipp modified the milestones: 5.12, 5.12 tmp Feb 2, 2024
@marcphilipp marcphilipp self-assigned this Mar 9, 2024
*/
@API(status = EXPERIMENTAL, since = "5.11")
default void publishFile(Path file) {
publishFile(file.getFileName().toString(), path -> Files.copy(file, path, REPLACE_EXISTING));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to move the file by default or at least provide an option to do so.

@@ -77,4 +82,36 @@ default void publishEntry(String value) {
this.publishEntry("value", value);
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a way to "add" a remote file, e.g., via a URI, would be useful.

}

private final LocalDateTime timestamp = LocalDateTime.now();
private final Path file;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a Map<String, String> for metadata/annotations would be useful.
For example, you could store the URL for the screenshot's current page and other values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants