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

8843 more harvest tests #9227

Merged
merged 12 commits into from
Dec 15, 2022
Merged

8843 more harvest tests #9227

merged 12 commits into from
Dec 15, 2022

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Dec 13, 2022

What this PR does / why we need it:
Self-explanatory; see "note for reviewer" for more info.

Which issue(s) this PR closes:

Closes #8843

Special notes for your reviewer:

While these tests still don't cover absolutely everything harvesting-related in the application, with these in place we'll be testing much more than we were up until now.
The server-side tests test both the native Dataverse API for managing the OAI sets provided by the installation, and the OAI server functionality itself.
The client side tests class validates the native API for managing harvesting configurations and runs an actual harvest of a control set.
Note that in its current form this last test relies on an external service - specifically, it will attempt to harvest a control set of metadata served by demo.dataverse.org (made up of a few datasets that are kept on demo permanently, not subject to the normal expiration limits). This arrangement has some obvious drawbacks - the test will fail if demo is down or unreachable. I'm open to feedback on this, and we should simply see how often this is going to be a problem. But if it is a problem, there should be a way to make this test self-contained, by adding a simple OAI server configuration to dataverse-ansible that would host these control records under OAI, outside of Dataverse, but still on localhost. This test "OAI server" could be faked with as little as a few static files and a few more Apache rewrite rules. While I consider this out of scope for this PR, it would not take much effort if deemed necessary going forward.

Also, there's a couple of minor changes in the PR that are not directly related to the tests, specifically it upgrades the application with the RC2 release of Oliver's new XOAI library, from the RC1 that was used in the original "reorganize the XOAI libs" PR.

Suggestions on how to test this:

Confirming that the automated jenkins tests pass is probably all the QA it needs. The tests rely on a couple of custom JVM options. Don helped add these to the Jenkins configuration (many thanks!!), so I'm hoping that it will just work... but I'll keep an eye on it.

edit: I can't think of any further QA of this, aside from maybe trying yet another Jenkins run by hand. But please see if you can think of anything else and let me know if you have any questions/need anything from me.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

@coveralls
Copy link

coveralls commented Dec 13, 2022

Coverage Status

Coverage increased (+0.004%) to 19.998% when pulling 7fa7836 on 8843-more-harvest-tests into 6c87b39 on develop.

@landreev
Copy link
Contributor Author

Oh no, did the Jenkins run get killed by the scheduled shutdown?

@landreev
Copy link
Contributor Author

Something is indeed failing, will figure it out tomorrow.

@pdurbin
Copy link
Member

pdurbin commented Dec 14, 2022

Also, there's a couple of minor changes in the PR that are not directly related to the tests, specifically it upgrades the application with the RC2 release of Oliver's new XOAI library, from the RC1 that was used in the original "reorganize the XOAI libs" PR.

Here's the diff:

gdcc/xoai@xoai-5.0.0-RC1...xoai-5.0.0-RC2

Something is indeed failing, will figure it out tomorrow.

Yes, this:

ava.lang.AssertionError: Last harvest not reported a success expected: but was:
at edu.harvard.iq.dataverse.api.HarvestingClientsIT.testHarvestingClientRun(HarvestingClientsIT.java:229)

Thanks for taking a look, @landreev

For a quick glance at the PR, here's my take:

  • I'm very excited to have all these new API tests. (We don't usually talk about harvesting like it's an API, but it is, obviously.)
  • There are code changes in here too but they are minimal and seem safe.
  • Yes, it's suboptimal to rely on demo being up for these tests to run but perhaps we can try to repoint these tests to somewhere else (a server we spin up?) in the future.
  • Overall, great stuff! I'm happy to add an official review once the tests are passing.

…in the wait for an async. operation + some extra logging (#8843)
@mreekie
Copy link

mreekie commented Dec 14, 2022

Counting as part of Dec 1 sprint. Will be finished out today.
(Mike screwed up the sprint ending. Had expected sprint to finish COB today)

@landreev
Copy link
Contributor Author

The test has passed after a slight tweak in the sleep-and-wait sequence. But let me run it a few more times to make sure.

@landreev
Copy link
Contributor Author

OK, it passed 3 times in a row now. It looks like it just needed a brief moment between starting an asynchronous operation (the harvest) and the first check.

@landreev landreev removed their assignment Dec 14, 2022
@pdurbin pdurbin self-assigned this Dec 14, 2022
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.

Hooray for tests! Approved! Off to QA.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for Review to QA ✅ Dec 14, 2022
@pdurbin pdurbin removed their assignment Dec 14, 2022
@mreekie
Copy link

mreekie commented Dec 14, 2022

added to sprint Dec 15, 2022

@mreekie mreekie added the Size: 10 A percentage of a sprint. 7 hours. label Dec 15, 2022
@mreekie
Copy link

mreekie commented Dec 15, 2022

I just added a totally arbitrary size to this just so that it represents a unit of work for this new sprint. I arbitrarily sized it as a 10.

@kcondon kcondon self-assigned this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 10 A percentage of a sprint. 7 hours.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Expand the suite of automated tests of the Harvesting functionality
5 participants