Provide a TestUtils class in the main artifact #70
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SciJava common is already used by dozens of libraries (and transitively by hundreds of software components). Besides being a powerful software library, SciJava common also sets helpful standards e.g. for unit testing. Following the DRY principle (Don't Repeat Yourself), let's offer the TestUtils class -- which turned out useful for testing software in the SciJava ecosystem -- as part of the standard library. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
MiniMaven already used a fork of SciJava common's TestUtils class, and even enhanced the naming of the temporary directories. Let's benefit from those improvements everywhere. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The benefits are obvious: should Jenkins report failures in one of these tests, we can easily inspect those temporary directories because they are now written into the target/ directory (which Jenkins makes visible, in contrast to /tmp/). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Contributor
Author
|
@ctrueden could you give this a quick look-over, please? |
Member
|
Apart from my comment above, looks excellent! |
dscho
added a commit
that referenced
this pull request
May 23, 2014
Curtis Rueden offered reluctance to fall back to creating tests in /tmp/ when it was determined that the tests are run in in unknown location. This developer agrees. Therefore, let's just throw an Exception in unknown cases. That way, we can add support for other test scenarios and call those changes "bug fixes" rather than "backwards-incompatible changes". For details, see the pull request corresponding to this topic branch: #70 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Curtis Rueden offered reluctance to fall back to creating tests in /tmp/ when it was determined that the tests are run in in unknown location. This developer agrees. Therefore, let's test carefully that we found some sensible location close to the .class files, handling unit tests run via Maven extra nicely. For details, see the pull request corresponding to this topic branch: #70 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
If File.exists() returns true, it might be a regular file being in the way, after all... Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
added a commit
that referenced
this pull request
May 23, 2014
Provide a TestUtils class in the main artifact
Contributor
Author
|
Thank you, @ctrueden, for your incredibly helpful comments! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
TestUtilsclass was already forked inij1-patcherandMiniMaven. The latter also had enhancements that more users might want to benefit from. Let's port them back intoscijava-commonand drag theTestUtilsout ofsrc/test/java/'s shadows intosrc/main/java/'s lime light.This addresses #63.