Skip to content

for discussion, run macro via imagej api#70

Open
k-dominik wants to merge 1 commit intomainfrom
add-macro-regression-tests
Open

for discussion, run macro via imagej api#70
k-dominik wants to merge 1 commit intomainfrom
add-macro-regression-tests

Conversation

@k-dominik
Copy link
Copy Markdown
Contributor

@k-dominik k-dominik commented Sep 10, 2020

I think it would be nice running macros as integration tests. But I'm not sure what the best way would be to do it. It works using the ImageJ api, but it will fire up a gui (so CI will probably fail, or we need to xvfb-run it CI seems cool with it).
My vision would be to also install ilastik on CI and run integration tests for the other workflows, too. Any opinions?

cc @emilmelnikov @m-novikov @Tomaz-Vieira

@k-dominik k-dominik marked this pull request as draft September 10, 2020 16:12
@emilmelnikov
Copy link
Copy Markdown
Member

I think it should be somehow possible to run an ImageJ macro in the headless mode.

@k-dominik
Copy link
Copy Markdown
Contributor Author

I think it should be somehow possible to run an ImageJ macro in the headless mode.

So from command line, no problem...

fiji-executable --headless -macro path/to/macro.ijm

but from code, I didn't manage =D

@m-novikov
Copy link
Copy Markdown
Contributor

I think it should be somehow possible to run an ImageJ macro in the headless mode.

I think in this case it's nice to use ImageJ api, because we can query for ImageJ state after. Otherwise macro would need to contain some print statements to list image and dimensions.

@k-dominik k-dominik marked this pull request as ready for review November 23, 2020 10:03
@k-dominik
Copy link
Copy Markdown
Contributor Author

okay, moving forward with this, as I want to add some more tests this week!

@emilmelnikov
Copy link
Copy Markdown
Member

emilmelnikov commented Nov 23, 2020

I'm not sure we really need integration tests here. Macros just pass arguments to our ImageJ commands, so we can reduce macro regression testing to command regression testing. But our commands just build command-line options for the ilastik executable, so instead of running ilastik we can check whether one list of strings equals to an expected list of strings. We'll need to move the code from commands to external functions/classes though.

@k-dominik
Copy link
Copy Markdown
Contributor Author

I'm not sure we really need integration tests here. Macros just pass arguments to our ImageJ commands, so we can reduce macro regression testing to command regression testing. But our commands just build command-line options for the ilastik executable, so instead of running ilastik we can check whether one list of strings equals to an expected list of strings. We'll need to move the code from commands to external functions/classes though.

sure, although this is not entirely true. Running the plugin usually entails writing the data to one or multiple temporary files, before running ilastik. But it's probably enough to check that the file exists and the expected command line arguments are passed. This introduces of course a second source of truth that needs to be kept in sync. I could not, e.g. always test it against the current release of ilastik (or multiple versions) and know it will work.

After all downloading a 400mb zip is not so hard imo. And at least it would take away the burden of keeping things in sync.

@k-dominik
Copy link
Copy Markdown
Contributor Author

I thought a bit more about it and I think I was mixing things.

  1. Testing validity of passed command line arguments against an agreed-upon cli
  2. Testing backwards compatibility with ilastik

these should be separate things.

@emilmelnikov
Copy link
Copy Markdown
Member

I think backwards compatibility tests should be in the ilastik repo. After all, it's not only ilastik4ij that relies on that. Same for the CLI arguments: we can't really change them without potentially breaking someone else's scripts. So, I think it's OK to duplicate assertions about CLI arguments here, it's not a whole lot of duplication anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants