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

Upgrade JUnit #2940

Closed
pdurbin opened this issue Feb 10, 2016 · 20 comments · Fixed by #5071
Closed

Upgrade JUnit #2940

pdurbin opened this issue Feb 10, 2016 · 20 comments · Fixed by #5071
Assignees
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Type: Suggestion an idea

Comments

@pdurbin
Copy link
Member

pdurbin commented Feb 10, 2016

I just realized that there's no Javadoc for the version of JUnit we're using (4.8.1): http://search.maven.org/#search|gav|1|g%3A%22junit%22%20AND%20a%3A%22junit%22

It looks like I added that version a loooong time ago in 66dd604 so I doubt anyone care which version we upgrade to as long as it works.

the_central_repository_search_engine_-_2016-02-10_09 39 46

@pdurbin
Copy link
Member Author

pdurbin commented Jun 28, 2017

Meh, we can live without Javadoc for Junit. It's pretty straightforward.

@pdurbin
Copy link
Member Author

pdurbin commented Sep 19, 2018

Reopening and assigning to @poikilotherm as discussed at http://irclog.iq.harvard.edu/dataverse/2018-09-19 and moving to "Community Dev" at https://waffle.io/IQSS/dataverse . Thanks!!

@poikilotherm
Copy link
Contributor

Will try to upgrade to latest JUnit 4.12, to have org.junit.ClassRule available to use S3Mock as JUnit4 Rule.

And of course it always makes sense to have his rather old library updated.

Crossing fingers nothing blows up... 🤞

@matthew-a-dunlap
Copy link
Contributor

matthew-a-dunlap commented Sep 19, 2018

@poikilotherm that's great about upgrading! What sort of maintenance cost is associated with S3Mock? I'm very excited to have that mocked out for better testing but am somewhat weary if the docker-maven-plugin is heavy.

@poikilotherm
Copy link
Contributor

poikilotherm commented Sep 19, 2018

S3Mock can be run from a JUnit 4 or 5 rule, thus there is no need for a Docker based unit test... Thus should be lightweight.

Speaking of JUnit 4 and 5: do you guys see any reason why not to adapt JUnit 5? New tests could be done with that and all existing ones based on 4.x can run unchanged and in parallel. (This was one of the goals by the JUnit team...)

@matthew-a-dunlap
Copy link
Contributor

@poikilotherm Sounds good, I think I may have misread the readme.

In terms of upgrading, going to the latest would be great! We just want to ensure our old tests function as before.

@pdurbin
Copy link
Member Author

pdurbin commented Sep 19, 2018

Sure, if you can get JUnit 5 working, that's fine. Up to you, @poikilotherm . Thanks for working on this!

@pdurbin
Copy link
Member Author

pdurbin commented Sep 19, 2018

@poikilotherm I just noticed that you said something about running JUnit 4 and 5 in parallel at http://irclog.iq.harvard.edu/dataverse/2018-09-19#i_72801 (thanks for pointing this out @pameyer ). We should probably pick one or the other for simplicity. If the easiest path is to upgrade to 4.12, that's fine.

@oscardssmith
Copy link
Contributor

Junit5 has a way to do Junit4 style tests so it shouldn't be too bad.

poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 20, 2018
…mbined with JUnit 4.12 Vintage Engine.

This allows us to have both versions active in parallel and make the migration easier.

Fixed `IngestableDataCheckerTest.testTestSAVformat()` unit test, which was failing with 4.12.
This was not an engine fault but a mistake within the test case itself.
@pdurbin
Copy link
Member Author

pdurbin commented Sep 20, 2018

@poikilotherm I see you made pull request #5071 so I moved it to code review at https://waffle.io/IQSS/dataverse . I know it's a little strange but in general we try to keep the discussion in the issue rather than the pull request so I'm going to comment here on what you wrote about HarvestingServerIT.testOaiFunctionality in the pull request. @pameyer already has some ideas about how to fix it in pull request #5070. And I see that we're discussing in pull requests there too so I guess we're not very consistent about where to discuss stuff. Anyway, it sounds like there are some failing tests in your pull request so it shouldn't be merged yet but at least that harvesting test failure is new and has nothing to do with the JUnit upgrade. I hope this makes sense.

@poikilotherm
Copy link
Contributor

@pdurbin: sorry, I'm puzzled. All unit tests are running smoothly, there is just this IT test failing which seems unrelated to JUnit. So there should be no blocker from this, right?

I can stick to issues, no problem. 😄
Instead of adding that in the PR: I just added a commit with an update to Mockito, too (only used in VERY rare cases), to be "JUnit 5-ready" with this...

@pdurbin
Copy link
Member Author

pdurbin commented Sep 20, 2018

You're right, we could probably merge the pull requests independently but I guess my perspective on this is that we should always get the integration tests back to passing before making any major changes. So #5069 is a much higher priority in my view than this issue.

@poikilotherm
Copy link
Contributor

There might be a blocker for upgrading to JUnit 5 depening on decisions in #5068: Arquillian is JUnit 4 only as of writing this (they are discussing on supporting it in arquillian/arquillian-core#137).

I have not yet tested if using the Vintage engine is compatible with Arquillian as mentioned in that issue. If this is not possible, it might be needed to rollback this later on.

@pdurbin
Copy link
Member Author

pdurbin commented Sep 21, 2018

@poikilotherm ok, should we just upgrade to JUnit 4.12 for now? So that you can use S3Mock?

@poikilotherm
Copy link
Contributor

I will test it in a separate little test project using some simple Arquillian example and report back. IMHO it would be a good idea to be future proof with using JUnit 5...

@pdurbin
Copy link
Member Author

pdurbin commented Sep 22, 2018

That's how we started with REST Assured. One tiny file to exercise the API a bit. Our usage has grown quite a bit.

poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 24, 2018
…mbined with JUnit 4.12 Vintage Engine.

This allows us to have both versions active in parallel and make the migration easier.

Fixed `IngestableDataCheckerTest.testTestSAVformat()` unit test, which was failing with 4.12.
This was not an engine fault but a mistake within the test case itself.
This was referenced Sep 24, 2018
@poikilotherm
Copy link
Contributor

poikilotherm commented Sep 24, 2018

Hey @pdurbin,

I just walked through http://arquillian.org/guides/getting_started and extended the tutorial project to use JUnit 4 based Arquillian tests with the JUnit5 Vintage Engine.

And guess what: it just works... 🕺

Could you guys please review the PR and (hopefully) merge it? THX!

(Oh and while we're on it: could you assign this issue to me? That would be awesome, as I have to find it near the bottom of the longer "mentioned" list because of the issue date...)

@matthew-a-dunlap
Copy link
Contributor

This looks to be working well! The only caveat is that I had to completely blow away my .m2 folder before maven would grab the correct versions of things... but that's maven for you ¯_(ツ)_/¯

@matthew-a-dunlap
Copy link
Contributor

matthew-a-dunlap commented Sep 24, 2018

@poikilotherm fwiw we often use our waffle board to supplement the github issue UI: https://waffle.io/IQSS/dataverse

@matthew-a-dunlap matthew-a-dunlap removed their assignment Sep 24, 2018
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 25, 2018
…mbined with JUnit 4.12 Vintage Engine.

This allows us to have both versions active in parallel and make the migration easier.

Fixed `IngestableDataCheckerTest.testTestSAVformat()` unit test, which was failing with 4.12.
This was not an engine fault but a mistake within the test case itself.
@matthew-a-dunlap matthew-a-dunlap removed their assignment Sep 25, 2018
@kcondon kcondon assigned kcondon and unassigned poikilotherm Sep 26, 2018
kcondon added a commit that referenced this issue Sep 27, 2018
@poikilotherm
Copy link
Contributor

Thanks for merging @kcondon 👍

Wow, my first real contribution to Dataverse... 😄 Glad you guys merged it so fast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Type: Suggestion an idea
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants