Add a test framework for scripts #16

hadim · 2016-12-04T18:25:20Z

In continuation to #15 I propose this.

What do you think ?

hadim · 2016-12-04T18:26:11Z

hadim · 2016-12-04T23:56:57Z

I was thinking maybe I could use the .fake format such as "8bit-signed&pixelType=int8&axes=X,Y,Z,Channel,Time&lengths=1160,1160,3,5,7.fake"

ctrueden · 2016-12-05T12:51:50Z

We use the fake format for testing in many places. Note that as part of the SJC3 I/O work, it will change from a pseudoformat with .fake extension, to a URI-style protocol like testimage:// or something like that. But I'll try to preserve backwards compatibility.

imagejan

Nice work! You are currently just checking if the scripts don't fail, right? How about checking if they return the correct result as well?

Also, can you split the whitespace changes in the crop script into a separate commit (as they are unrelated to the test framework)?

imagejan · 2016-12-05T14:17:54Z

src/test/java/net/imagej/scripting/AbstractScriptTest.java

+ *
+ * @author Hadrien Mary
+ */
+public class AbstractScriptTest {


abstract keyword missing (at least the class name suggests so)

imagejan · 2016-12-05T14:23:40Z

src/test/java/net/imagej/scripting/TestScript.java

+ *
+ * @author Hadrien Mary
+ */
+public class TestScript extends AbstractScriptTest {


In line with the current ImageJ conventions, and to be consistent with AbstractScriptTest, the class name should be ScriptTest or maybe some more specific ImageJ2ScriptTest, so there can also be DeconvolutionScriptTest etc.

IIRC, throughout the ImageJ universe (e.g. imagej-ops), the convention is to have an interface (i.e. interface ScriptTest) that is implemented by an abstract class that is then extended by a default implementation or several specific ones.

imagejan · 2016-12-05T14:24:24Z

src/test/java/net/imagej/scripting/TestScript.java

+
+      final ScriptModule m = scriptService.run(scriptFile, true).get();
+
+      final Dataset output = (Dataset) m.getReturnValue();


How about asserting that the output is not null?

hadim · 2016-12-06T00:52:22Z

Ok after a big fight with the API I have finally figured out how to test those scripts :-)

I am pretty happy with the result. Now everyone should easily be able to test scripts in this repo and that will increase a lot their robustness and usefulness. That was needed in my opinion by the fact they are available in the Script Editor. Such scripts should never throw an error in the Script Editor.

Once this is merged I invite everyone to write tests for their own scripts in this repo (it's super easy and quick to do).

Beside that I am happy to address any others comments you might have about this PR (please don't ask me to separate the commit that modify the scripts from the commit of the tests...).

hadim · 2016-12-06T01:01:52Z

Update : the tests pass without issue but to do that I had to add :

<dependency>
            <groupId>net.imagej</groupId>
            <artifactId>imagej</artifactId>
            <scope>test</scope>
            <type>jar</type>
        </dependency>

to the pom.xml which create a circular dependency and maven does not seem to like it a lot :-(

Exact error is :

Rule 6: org.apache.maven.plugins.enforcer.BanCircularDependencies failed with message:
Circular Dependency found. Your project's groupId:artifactId combination must not exist in the list of direct or transitive dependencies.
  net.imagej:imagej-scripting

imagejan · 2016-12-06T09:00:57Z

Hm, I'm not sure how to best solve the circular dependency issue. Any scripts using net.imagej.Dataset will definitely need imagej as a (test/runtime) dependency.
I guess imagej-scripting could be removed from imagej's pom.xml, where it currently is a runtime dependency?!
@ctrueden we'll need your advice here again...

hadim · 2016-12-06T13:40:23Z

Exactly we are stuck because this repo needs ImageJ to run those scripts...

imagejan · 2016-12-06T13:48:16Z

Exactly we are stuck because this repo needs ImageJ to run those scripts...

Yes, but ImageJ doesn't require this repo, does it?

hadim · 2016-12-06T14:11:23Z

Well ImageJ ship with those scripts in the Script Editor... So we have to figure out which component of the ecosystem really needs this repo as a dep. Ideally that would be only the Script Editor repo correct ?

hadim · 2016-12-06T14:37:24Z

So script-editor is a scijava package (https://github.com/scijava/script-editor) so I guess it's not acceptable to add imagej-scripting to it.

Beside that imagej-scripting package is never called because of the auto discovery mechanism of resources/script_templates meaning that scripts will be added to script-editor whatever the package is.

So in theory the most obvious repo to add imagej-scripting is ImageJ at the end.

I realize is far from ideal but I propose to remove image-scripting from ImageJ repo and add it instead to imagej-ui-swing. The motivation is that imagej-scripting is only available to the user through an UI at the end. Because imagej-scripting does not depend on imagej-ui-swing that should solve the issue.

What do you think ? I am open to any others suggestions.

hadim · 2016-12-06T14:45:01Z

I confirm the trick works locally on my machine.

imagejan · 2016-12-06T16:07:29Z

Why does imagej need to depend on imagej-scripting at all? It's sufficient if scripting is present at runtime for the templates to be discovered. If it's missing, ImageJ will still run, no?

Maybe I'm missing something here, or is the dependency required for Jenkins to deploy the whole ImageJ application??

hadim · 2016-12-06T16:24:39Z

Exactly. I was thinking about Jenkins deployment. I think ImageJ contains all the deps that need to be shipped to build the final software.

hadim · 2016-12-06T16:27:22Z

So at the end we should find another way to ship imagej-scripting. The proposal made above is one of the way (probably not the best one).

bnorthan · 2016-12-08T16:03:15Z

@hadim I just got the latest code and ran into the "Circular Dependency Error", at this point I don't have anything to add to that discussion. However when this part is fixed, and the project is easy to build I will be happy to do further testing.

hadim · 2016-12-08T16:37:37Z

Yes it's because of the imagej dependency.

To make it work you'll have to remove imagej-scripting dep from the imagej pom.xml and use that version of imagej (remove the SNAPSHOT tag from imagej also).

i realise it's complicated but at the moment I have no other solutions than that.

Let's wait next week and see what @ctrueden has to say about that.

ctrueden · 2016-12-09T03:55:12Z

My quick 2c from a mobile devixe while on vacation: * imagej should have a runtime dep on imagej-scripting. * imagej-scripting should _not_ depend on imagej. It should depend on e.g. imagej-common where Dataset lives. * If imagej-scripting truly needs the net.imagej.ImageJ gateway (does it? I somehow doubt it), then we have a problem... which could be solved by adding another component like imagej-app which unions imagej + imagej-scripting and maybe other things. But I hope we can avoid this.

imagejan · 2016-12-09T10:40:39Z

imagej-scripting should not depend on imagej. It should depend on e.g. imagej-common where Dataset lives.

Thanks, @ctrueden, for that hint. Indeed, we don't need net.imagej.ImageJ in your scripts, @hadim. They can be replaced by the appropriate services:

# @OpService ops
# @DatasetService ds

# [...]

ops.run("...") # instead of ij.op().run()

output = ds.create(outputImg) # instead of ij.dataset().create()

I tested these changes and pushed a commit to the test-scripts branch. (@hadim you can also push topic branches directly on the main repo instead of your fork. If you then open a pull request, we can all modify it before merging by pushing to that topic branch.)

Do we actually need the DatasetService at all here? Can't you just output the Imgs themselves?

hadim · 2016-12-09T13:47:06Z

Well I understand that you don't want imagej dep in imagej-scripting but from a "scripting" point of view, scripts we put in this repo should not be restricted to a smaller API/libraries than the one available in the Script Editor just because of a pom.xml issue.

I am aware I can use DatasetService and OpService instead of ImageJ but I prefer to use ImageJ because it decreases the number of lines at the beginning of the script and I use it in all my scripts. Same is for Img, I know I could use it instead of Dataset but I prefer to have the same input for all my scripts and use Dataset because sometime I need a Dataset object.

imagej is the package that contain all the ImageJ dependencies and also the ImageJ class. Do these two things need to be in the same repo ? Would it be a possibility to move ImageJ class in imagej-common for example ? It's just a guess here and I probably don't know well enough the overall API to be able to make a good judgment.

Now if you guys don't see any alternative that make everyone happy I am gonna remove all the # @ImageJ ij.

bnorthan · 2016-12-09T14:04:24Z

@hadim these scripts should be structured so that they could be run in any scijava application, for example KNIME. So we need to consider what services/classes/structures are generic to scijava, and which ones are specific to ImageJ.

hadim · 2016-12-09T14:13:34Z

Fair enough, I understand the scijava application argument. Let's close this PR and open a new one with the test-scripts branch in this repo.

hadim · 2016-12-09T14:14:46Z

Here is the new PR : #17

ctrueden · 2016-12-09T14:39:21Z

Hadrien certainly has a point, and I agree that ideally use of ImageJ gateway should be okay and even encouraged for easy scripting. That was one main reason for its existence after all. I have several thoughts and options to suggest but cannot write them on mobile device now easily. I will respond in more detail next week.

hadim · 2016-12-09T15:49:58Z

Glad to hear :-)

So let's just merge #17 without any ij call in the scripts and open a new discussion for this ImageJ dep story.

hadim mentioned this pull request Dec 4, 2016

Test all the scripts (when possible) #15

Closed

imagejan requested changes Dec 5, 2016

View reviewed changes

hadim force-pushed the test-scripts branch 2 times, most recently from 7df1428 to 9724281 Compare December 6, 2016 00:12

Autoindent

5927927

hadim force-pushed the test-scripts branch 2 times, most recently from c30173a to 2ef01fc Compare December 6, 2016 00:15

Add a test framework for scripts + tests for ImageJ2 scripts

c217ff0

hadim force-pushed the test-scripts branch from 2ef01fc to c217ff0 Compare December 6, 2016 00:46

Update README to explain the test framework

fbcd108

hadim assigned hadim and unassigned hadim Dec 6, 2016

imagejan approved these changes Dec 6, 2016

View reviewed changes

hadim closed this Dec 9, 2016

hadim deleted the test-scripts branch December 9, 2016 14:13

hadim mentioned this pull request Dec 9, 2016

Add a test framework for scripts #17

Merged

hadim mentioned this pull request Dec 9, 2016

Including ImageJ gateway #18

Open


		final ScriptModule m = scriptService.run(scriptFile, true).get();

		final Dataset output = (Dataset) m.getReturnValue();

Add a test framework for scripts #16

Add a test framework for scripts #16

Uh oh!

Conversation

hadim commented Dec 4, 2016

Uh oh!

hadim commented Dec 4, 2016

Uh oh!

hadim commented Dec 4, 2016

Uh oh!

ctrueden commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imagejan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imagejan Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

imagejan Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

imagejan Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

hadim commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadim commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imagejan commented Dec 6, 2016

Uh oh!

hadim commented Dec 6, 2016

Uh oh!

imagejan commented Dec 6, 2016

Uh oh!

hadim commented Dec 6, 2016

Uh oh!

hadim commented Dec 6, 2016

Uh oh!

hadim commented Dec 6, 2016

Uh oh!

imagejan commented Dec 6, 2016

Uh oh!

hadim commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadim commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnorthan commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadim commented Dec 8, 2016

Uh oh!

ctrueden commented Dec 9, 2016 via email

Uh oh!

imagejan commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadim commented Dec 9, 2016

Uh oh!

bnorthan commented Dec 9, 2016

Uh oh!

hadim commented Dec 9, 2016

Uh oh!

hadim commented Dec 9, 2016

Uh oh!

ctrueden commented Dec 9, 2016 via email

Uh oh!

hadim commented Dec 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ctrueden commented Dec 5, 2016 •

edited

Loading

imagejan left a comment •

edited

Loading

hadim commented Dec 6, 2016 •

edited

Loading

hadim commented Dec 6, 2016 •

edited

Loading

hadim commented Dec 6, 2016 •

edited

Loading

hadim commented Dec 6, 2016 •

edited

Loading

bnorthan commented Dec 8, 2016 •

edited

Loading

imagejan commented Dec 9, 2016 •

edited

Loading