Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integration tests #70

Merged
merged 16 commits into from Feb 27, 2018

Conversation

@awalter17
Copy link
Contributor

commented Dec 14, 2017

This builds upon #69 and should not be merged until that is! This PR simply adds the integration tests. And at this time more integration tests are needed.

@awalter17 awalter17 force-pushed the integration-test branch from 9090500 to 4fc970a Dec 15, 2017
@awalter17

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

Huzzah green check mark! 🎉

Now I need to add tests for importing/exporting images and tables. Then probably on another branch, based on the rois branch, add tests for ROIs.

Thanks @joshmoore!

@awalter17 awalter17 force-pushed the integration-test branch from 179cb2a to 18cab2c Jan 9, 2018
@awalter17 awalter17 force-pushed the integration-test branch from 8c04145 to 18cab2c Jan 24, 2018
@awalter17

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

I've moved benchmark tests to a separate branch, and made them a separate issue (#72).

When building the containers locally vs. on travis, my table is created with a different ID. This is a similar issue to what's being seen here.

To avoid this we may want to get rid of my python script (this commit fd5a6f0), and instead favor a database dump.

@joshmoore as for where this database dump should live (this repo? As a gist? In nexus? As a docker
container? etc.)?

I don't think this repo is the best place for it, because hopefully this database will be more general than just imagej-omero test cases. On some level having a docker container is appealing. We could make an omero-test-db, which is based on the postgres docker image. That being said, I'm not certain that's the best move either. Thoughts @ctrueden, @dominikl, @sbesson, @jburel?

@awalter17

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

What needs to be done before this branch can be merged:

  • #69 needs to be merged - @joshmoore if you're happy with it, I can merge it
  • Modify imagej-omero master to depend on omero-client 5.4, not 5.3 - this is something I need to do, and will hopefully do soon
  • Decide what to do about the database dump (see previous comment)
    • we could just keep the python script, but I think the database dump would be useful for bench marking because we'd want images, tables, rois of varying complexity/size

Please let me know if you have any questions or concerns!

@joshmoore

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

gh-69 merged. See comment there.

@joshmoore

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Currently reading https://github.com/muthu-r/horcrux re: storage.

@ctrueden

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Containerization of data volumes? I guess it's just a local mutable cache layer, but still... Crazy!

@joshmoore

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

I've come away from a docker-specific solution in favor of teaching omero-test-infra ( see ome/omero-test-infra#8 ) how to persist volumes to disk. This may be a building block for then doing more sophisticated things later.

if [ ! -d ".omeroci" ];
then
mkdir .omeroci
echo "#!/bin/sh" > .omeroci/test-data

This comment has been minimized.

This comment has been minimized.

Copy link
@awalter17

awalter17 Feb 23, 2018

Author Contributor

@joshmoore I just saw your PR in openmicroscopy/omero-test-infra, thanks for doing that! I've made a temporary commit using those changes just to ensure they work on travis. And they do (yay!).

@awalter17 awalter17 force-pushed the integration-test branch 2 times, most recently from 8c4bfac to 8164957 Feb 22, 2018
@joshmoore

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Pushed v0.3.1. Thanks for the testing @awalter17

@awalter17 awalter17 force-pushed the integration-test branch 2 times, most recently from 301d345 to 1107d1d Feb 27, 2018
awalter17 added 4 commits Feb 27, 2018
This commit modifies the pom to no longer depend on omero-client, but
instead use omero-blitz. Additionally, this commit sets the enforcer to
be skipped due to a number of duplicate classes.
This removes testng and various logging dependencies, which lead to
extraneous logging when building.
By default the integration tests will not run. To run the integration
tests use:

mvn failsafe:integration-test failsafe:verify -DskipITs=false

The verify goal is necessary to ensure that the build fails when all the
integration tests do not pass.
@awalter17 awalter17 force-pushed the integration-test branch from 1107d1d to 51aadf7 Feb 27, 2018
awalter17 and others added 6 commits Dec 14, 2017
This splits the omero-test-infra logic out into a new
.travis/test.sh script which can be called indepdently.
It's designed to allow travis to download an archive
and restore the state of OMERO.

See ome/omero-test-infra#8
awalter17 added 5 commits Feb 20, 2018
This is no longer needed, since we're now using a database backup to
generate the data.
The purpose of these tests is to ensure a new ID is assigned to the
objects, even though they originated from OMERO. Currently
  imagej-omero does not update objects on the server, it only uploads
  new versions.
@awalter17 awalter17 force-pushed the integration-test branch from 51aadf7 to 3739d9a Feb 27, 2018
Accomplished by:
  mvn license:update-file-header
@awalter17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2018

Thanks @joshmoore! I've updated test.sh to use v0.3.1.

Alright, I believe this PR is ready to merge! Note that this PR adds integration tests for images and tables, and updates the OMERO dependency version to 5.4.

All commits in this PR compile with passing tests (not integration tests).

@joshmoore, @ctrueden please let me know if you have any questions or if you'd like any changes.

@ctrueden ctrueden changed the title Add integration tests (DO NOT MERGE) Add integration tests Feb 27, 2018
@ctrueden

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

@awalter17 We need to fix the enforcer configuration. Rather than skipping the enforcer, we should add an enforcer configuration block as described in the banDuplicateClasses rule documentation.

  • If the duplicate classes can be avoided with dependency exclusions, that is the preferred solution. But if not, the enforcer rule is the way to go.
  • If we expect much downstream code to depend on net.imagej:imagej-omero then we probably want to declare this duplicate classes block in the pom-scijava-base component, so that every single downstream component does not need to redeclare the configuration.

Ideally we would also push the blitz Maven configuration's version properties and exclusions up to pom-scijava, cut a new pom-scijava, and then update this component to depend on the new pom-scijava version.

I'm willing to wait on all of this until after the PR merges though.

@ctrueden ctrueden merged commit 853e1cc into master Feb 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ctrueden ctrueden deleted the integration-test branch Feb 27, 2018
@ctrueden

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

@awalter17 I merged it, so you can't do any more rebases. 😜

Let me know if you have any questions about the Maven stuff.

@joshmoore

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Very exciting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.