JUnit 5 support - A new junitlauncher task #60
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Please ignore the Jenkins failures on this PR. I'll sort out that part in a separate discussion. |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| try { | ||
| this.sysErrFilePath = Files.createTempFile(null, "syserr"); | ||
| this.sysErrFilePath.toFile().deleteOnExit(); | ||
| this.sysErrStream = Files.newOutputStream(this.sysOutFilePath); |
There was a problem hiding this comment.
copy-paste error, you want that to be sysErrFilePath :-)
| try (final InputStream is = Files.newInputStream(path)) { | ||
| while ((numBytes = is.read(content)) != -1) { | ||
| writer.write(new String(content, 0, numBytes)); | ||
| } |
There was a problem hiding this comment.
I don't think this is going to work reliably. The read may have split a multi-byte sequence at the end of content and then creating a string from it is going to break. Is there any reason you want to use a stream when reading the temporary file rather than a reader?
There was a problem hiding this comment.
You are right, good catch! I hadn't considered that scenario. I have now switched to using a reader instead.
| } catch (Exception e) { | ||
| // ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes, make sense. Fixed.
|
@bodewig, Thank you very much for the review. Based on the review comments, I have updated the PR with additional separate commits (for easier reference). I'll be squashing all these commits together during merge. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| * task. This classpath will then be used for execution of the tests | ||
| */ | ||
| public Path createClassPath() { | ||
| this.classPath = new Path(getProject()); |
There was a problem hiding this comment.
if users specify multiple nested classpath elements, only the last one will be used. You may want to return an already created this.classpath or move on to an add method and collect the paths inside of a wrapper Path.
There was a problem hiding this comment.
You are right. I hadn't taken that into account. I have updated the PR to change this part of the code.
| */ | ||
| public SingleTestClass createTest() { | ||
| final SingleTestClass test = new SingleTestClass(); | ||
| this.preConfigure(test); |
There was a problem hiding this comment.
create* is called before the attribute setters, If you call preconfigure here the test won't see the actual values set at the task level.
addConfigured would get called after the attribute setters, alternatively you need to defer pre-configuration until execute is called. With both approaches you need to ensure you don't overwrite the test's explicit configuration with values from the task, of course.
Same for createTestClasses.
There was a problem hiding this comment.
Thank you, for catching this. I went back and read the Ant manual about writing custom tasks (which I had skipped the first time around) and now understand this logic of execution better. I have updated the PR to fix this and use the addConfiguredXXX construct and also made sure my preConfigure implementation doesn't overwrite values that might have been specified on the nested elements.
|
|
||
| private static void safeClose(final Closeable... closeables) { | ||
| for (final Closeable closeable : closeables) { | ||
| try { |
| final byte[] data = new byte[1024]; | ||
| try { | ||
| while ((numRead = this.sourceStream.read(data)) != -1) { | ||
| final byte[] copy = new byte[numRead]; |
| @Override | ||
| public void run() { | ||
| if (this.resultFormatters.isEmpty()) { | ||
| // no one to feed the stream content to |
There was a problem hiding this comment.
I believe this may cause the PipedOutputStream to consider the PipedInputStream "broken" soon and throw on any write operation.
There was a problem hiding this comment.
This check is actually "dead code", in the sense that this will never be true. I wanted to avoid running these threads when there's no result formatter interested in the sysout/syserr content. But that check obviously needs to happen before the thread is even created and in fact, there's already such a check in the trySwitchSysOut and trySwitchSysErr methods (the place where this thread gets created).
So I've now updated the PR to remove this check.
…for reading sysout/syserr content
…s.close() + Remove unnecessary check in the streamer thread
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
…ized limited buffer, before switching to a file based store, if the size limit is exceeded
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| // the buffer capacity can't hold this incoming data, so this | ||
| // incoming data hasn't been transferred to the buffer. let's | ||
| // now fall back to a file store | ||
| this.usingFileStore = true; |
There was a problem hiding this comment.
maybe defer setting the flag until storeToFile below succeeds?
Also, is there any chance this method could be invoked concurrently? I haven't fully checked every execution path so maybe you are synchronizing somewhere. Tests could spawn new threads you are not aware of and these threads could be writing to syserr/out concurrently.
There was a problem hiding this comment.
maybe defer setting the flag until storeToFile below succeeds?
I did actually think of it while coding this part. The reason I decided to go this way, was to make sure that as soon as the memory size limit is reached, I prefered it to be better to no more use the in memory store even if the file store cannot be created/used.
That would imply that the subsequent syserr/sysout message will be lost but at least that won't lead to the in memory store going past the limit that's set.
The alternate approach would be to let the in-memory store be used even past its limit (and not lose any sysout/syserr content) if the file store cannot be created for whatever reason. If that feels more better approach to take, I can switch this logic in here.
There was a problem hiding this comment.
Also, is there any chance this method could be invoked concurrently? I haven't fully checked every execution path so maybe you are synchronizing somewhere. Tests could spawn new threads you are not aware of and these threads could be writing to syserr/out concurrently.
One thing I didn't note in the implementation comments is that the sysOutAvailable and sysErrAvailable on these result formatter implementations will always be invoked by a single thread - the thread that we create and control SysOutErrContentDeliverer. Multiple threads write out to our redirected sysout/syserr stream which then gets read by the SysOutErrStreamReader thread which then hands off that read data to the SysOutErrContentDeliverer thread to have it delivered to these result formatters.
|
|
||
| private FileOutputStream createFileStore() throws IOException { | ||
| this.filePath = Files.createTempFile(null, this.tmpFileSuffix); | ||
| this.filePath.toFile().deleteOnExit(); |
There was a problem hiding this comment.
FileUtils.createTempFile? Probably not worth it.
There was a problem hiding this comment.
I hadn't noticed that API. I think it makes sense to use it for consistency.
| sysErrStreamer.start(); | ||
| return Optional.of(new SwitchedStreamHandle(pipedOutputStream, streamer)); | ||
| } | ||
|
|
There was a problem hiding this comment.
you could probably DRY up the two methods a little by extracting the common logic.
There was a problem hiding this comment.
Yes makes sense, I'll clean up this part a bit.
|
Nice work! I don't see any reason not to merge this PR. |
|
Thank you Stefan for the thorough review. I'll clean up the commits to squash them into one and do these minor final changes before merging. |
|
This is now merged upstream. Closing this PR. Thanks Stefan for the extensive review. |
This is the initial working version of a new
junitlaunchertask that support using JUnit 5 framework for testing, within Ant build files. The commit in this PR is the initial set of goals that I had in mind for the first release of this task.The manual for this task can be (temporarily) found at https://home.apache.org/~jaikiran/temp_workspace/manual/Tasks/junitlauncher.html
The dev list discussion about this PR can be found at https://www.mail-archive.com/dev@ant.apache.org/msg46656.html