Add a test framework for scripts

[hadim]

In continuation to https://github.com/imagej/imagej-scripting/issues/15 I propose this.

What do you think ?

[hadim]

ping @bnorthan @imagejan @ctrueden

[hadim]

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]

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.

[hadim]

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]

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]

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]

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

[imagejan]

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

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

[hadim]

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]

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]

I confirm the trick works locally on my machine.

[imagejan]

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]

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]

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]

@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]

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]

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]

• 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]

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]

@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]

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]

Here is the new PR : https://github.com/imagej/imagej-scripting/pull/17

[ctrueden]

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]

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.