-
Notifications
You must be signed in to change notification settings - Fork 501
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
8843 more harvest tests #9227
Conversation
…) functionality of ListIdentifiers and ListRecords (#8843)
Oh no, did the Jenkins run get killed by the scheduled shutdown? |
Something is indeed failing, will figure it out tomorrow. |
Here's the diff: gdcc/xoai@xoai-5.0.0-RC1...xoai-5.0.0-RC2
Yes, this: ava.lang.AssertionError: Last harvest not reported a success expected: but was: Thanks for taking a look, @landreev For a quick glance at the PR, here's my take:
|
…in the wait for an async. operation + some extra logging (#8843)
Counting as part of Dec 1 sprint. Will be finished out today. |
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. |
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. |
There was a problem hiding this 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.
added to sprint 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. |
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: