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

6799 - Adding support for Testcontainers #6800

Merged
merged 3 commits into from
Apr 9, 2020

Conversation

poikilotherm
Copy link
Contributor

What this PR does / why we need it:
This PR introduces basic support for Testcontainers usage in integration tests using JUnit 5, which was mentioned in a few places like #6783 and talked about with @scolapasta and @qqmyers in a VC about #6694.

For now this is basically a docs change and a new Maven profile. Very basic, very "stay.out.of.my.way". Would be happy to rely on it for any PR on #6694 et al. Actually, integration testing some of authentication providers might be possible that way for the first time.

Which issue(s) this PR closes:
Closes #6799

Special notes for your reviewer:
Nada.

Suggestions on how to test this:

  1. Compiling and running tests should not fail when the profile isn't active. (Stay out of way)
  2. Running the profile via mvn -P tc verify should show 0 tests. (Obviously)
  3. Resulting WAR shall not have anything of these deps included.

Does this PR introduce a user interface change?:
Nada.

Is there a release notes update needed for this change?:
Depends. Maybe. Testcontainers is a huge and great thing, it might be neat to give @bsideup @kiview and @rnorth a shoutout when we really start to use this.

Additional documentation:
Provided in the PR.

@coveralls
Copy link

coveralls commented Apr 8, 2020

Coverage Status

Coverage remained the same at 19.661% when pulling 249be36 on poikilotherm:6799-testcontainers into d04d09c on IQSS:develop.

@pdurbin pdurbin self-assigned this Apr 8, 2020
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm excited about the kind of testing that Testcontainers promises to give us. I added a couple questions about pom.xml changes. Thanks for spearheading this effort, @poikilotherm !

@@ -765,6 +784,7 @@
<configuration>
<!-- testsToExclude come from the profile-->
<excludedGroups>${testsToExclude}</excludedGroups>
<skip>${skipUnitTests}</skip>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? It's <skip> instead of the <skipUnitTests> seen elsewhere? I think I'd rather see a smaller diff with this line removed. I would also suggest removing the <skipUnitTests>false</skipUnitTests> property above. I'm happy to talk this out in IRC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poikilotherm and I talked this out. It's all set. Please see my summary at #6800 (review)

doc/sphinx-guides/source/developers/testing.rst Outdated Show resolved Hide resolved
@pdurbin pdurbin assigned poikilotherm and unassigned pdurbin Apr 8, 2020
capitalize a word to match our style
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. @poikilotherm and I just talked out the skip unit tests thing. The long version is at http://irclog.iq.harvard.edu/dataverse/2020-04-08#i_121928

The short version "the only way to skip ONLY the unit tests is by switching them off via configuration" with the goal of NOT running unit tests (which get run all the time) when using Testcontainers. We don't want people hacking on Testcontainer tests to be bogged down by the time it takes unit tests to run. Also "skip" is legit in pom.xml.

I didn't run the code at all or test the new "tc" profile but @poikilotherm included instructions for doing so.

This pull request lays some groundwork. I'm looking forward to the future pull requests that make use of Testcontainers! 🎉

@kcondon kcondon self-assigned this Apr 9, 2020
@kcondon kcondon merged commit e5413e2 into IQSS:develop Apr 9, 2020
@djbrooke djbrooke added this to the Dataverse 5 milestone Apr 10, 2020
@poikilotherm poikilotherm deleted the 6799-testcontainers branch August 10, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Testcontainers support for more integration tests
5 participants