-
Notifications
You must be signed in to change notification settings - Fork 486
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
Comments
Meh, we can live without Javadoc for Junit. It's pretty straightforward. |
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!! |
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... 🤞 |
@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. |
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...) |
@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. |
Sure, if you can get JUnit 5 working, that's fine. Up to you, @poikilotherm . Thanks for working on this! |
@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. |
Junit5 has a way to do Junit4 style tests so it shouldn't be too bad. |
…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.
@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. |
@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. 😄 |
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. |
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. |
@poikilotherm ok, should we just upgrade to JUnit 4.12 for now? So that you can use S3Mock? |
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... |
That's how we started with REST Assured. One tiny file to exercise the API a bit. Our usage has grown quite a bit. |
…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.
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...) |
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 ¯_(ツ)_/¯ |
@poikilotherm fwiw we often use our waffle board to supplement the github issue UI: https://waffle.io/IQSS/dataverse |
…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.
Fixing #2940. Upgrading to use JUnit 5
Thanks for merging @kcondon 👍 Wow, my first real contribution to Dataverse... 😄 Glad you guys merged it so fast. |
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 text was updated successfully, but these errors were encountered: