Skip to content

Correct Matlab tests and test-matlab target#2989

Closed
rleigh-codelibre wants to merge 2 commits intoome:developfrom
rleigh-codelibre:matlab-tests
Closed

Correct Matlab tests and test-matlab target#2989
rleigh-codelibre wants to merge 2 commits intoome:developfrom
rleigh-codelibre:matlab-tests

Conversation

@rleigh-codelibre
Copy link
Contributor

@rleigh-codelibre rleigh-codelibre commented Nov 2, 2017

The ant target has been broken since the bio-formats component was renamed to formats-gpl. This change fixes the ant target paths so the tests can run with the correct environment.

The tests didn't previously run on Windows due to using : as a path separator. I've fixed this, and also made it work when MATLABPATH is unset so it doesn't contain a leading separator (which Matlab doesn't like).

The unit tests were also broken with recent Matlab versions due to changes in the Matlab TestCase and related classes. This change updates the tests to use the new way of doing things (≥2013b) using annotations for setup, teardown and test methods and the new ways for asserting things.

Most of the changes are simple mechanical changes. The only bit I'm not entirely sure of are the use of javaaddpath in TestBfCheckJavaPath.m. Needed to make the tests pass, but it may be that they would pass if run in a modified environment with bioformats_package in the same location as the bfmatlab files so that bfCheckJavaPath would work as intended; it looks like this is being tested, but won't work as expected in the test environment? It's not completely clear to me what the original expectations and suppositions were, and if they are unchanged.

Testing:

For each matlab version, set the JVM memory size to a large enough size. Also, run feature('DefaultCharacterSet','UTF8') to force it to use UTF-8 [may need adding to startup.m on non-Linux platforms]. Then run ant matlab-test to build bioformats and run the matlab tests.

@sbesson
Copy link
Member

sbesson commented Nov 3, 2017

Unfortunately, the changes break the daily CI job which expects the runxunit command to generate an XML output file that can be parsed by the job. More adjustements are required and need to be discussed. Excluding for now.

@sbesson sbesson added the exclude label Nov 3, 2017
@rleigh-codelibre
Copy link
Contributor Author

rleigh-codelibre commented Nov 3, 2017

@sbesson I've pushed a commit which adds a test-matlab-xunit target with a test.matlab.file property to specify the result file. This runs and creates the result file. But needs an additional tweak somewhere because it's not finding the jar for some reason. Would something along these lines be OK?

It's failing because I added the test dir to the matlab path; also fails if I don't add it to the path and set the working directory to the same location. I could always put that script somewhere else? So if I can invoke the logic in that script, it should simply be a matter of adjusting the jenkins job to use the ant target with the property.

@sbesson
Copy link
Member

sbesson commented Nov 15, 2017

Sorry for the delay in answering. Main caveat of 5112354 as far as I can see is that it's using the built in XMLPlugin which was introduced in MATLAB R2015b, while our CI infrastructure still uses R2013b as the installed version.

If I recall correctly, this is the primary reason why the matlab-xunit framework was selected back then to run unit tests and collect the output in an XML format.

Reviewing this is certainly a possibility if we want to directly consume the built-in framework. However, for all the reasons exposed above, this body of work is likely more than a simple adjustment of a job and minimally requires some team agreement on a set of questions:

  • which minimal version of MATLAB should we target for the base testing?
  • who should be leading the infrastructure work? what is the driver/timeline?
  • should these changes be carried out across the stack i.e. should the OMERO MATLAB unit tests be reviewed jointly?

@rleigh-codelibre
Copy link
Contributor Author

Regarding a minimal version, I think this depends upon the amount of effort we can dedicate to testing. How many versions can we realistically support at once? My testing showed up a problem with 2017a and Java 7 which we had not (to my knowledge) noticed before; if we want to properly test every supported version, I think this would suggest a limited number of versions, such as 2016 and 2017 versions only (as an example). How many platforms do we want to actively test?

Regarding infrastructure, which nodes currently have matlab installed? While the installer can't be easily automated (due to the licensing stuff), it won't be too awful to periodically install it as needed, but that might also be a factor in constraining the platforms/versions to test.

We could update the OMERO tests at the same time, but there's no need to directly couple the PRs since they could be done completely independently.

@joshmoore
Copy link
Member

With this currently excluded, closing & re-opening to see how appveyor fares with an un-bumped HEAD.

@joshmoore joshmoore closed this Nov 23, 2017
@joshmoore joshmoore reopened this Nov 23, 2017
@sbesson
Copy link
Member

sbesson commented Nov 28, 2017

Briefly discussed with @joshmoore. Given the current timelines and the infrastructure decisions and requirements, I do not see us having the capacity to move forward with this upgrade this year. Closing this PR and I will record the changes on the corresponding card so that we can reuse the work when we come back to it.

@rleigh-codelibre
Copy link
Contributor Author

This has been moved to microscopepony/bio-formats-matlab#3 and will not require re-opening here.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants