-
Notifications
You must be signed in to change notification settings - Fork 0
853 epic review of ezid api test coverage #16
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
base: main
Are you sure you want to change the base?
Conversation
…stead of being passed everywhere.
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.
Great work Scott!
A few things:
- "Create or update identifier" - can we add "with update_if_exists=yes" to the subject so we know what it is doing. Also, can we run this test item after "Update identifier" so it will show up before "reserve and delete identifier"
- We can remove checking background jobs from this script now
- How about we change the script and class name to something like verify ezid status
- The headless UI test is in the main but not this branch. Should we merge main to this branch first before merge it back to main? I am not sure.
- I got an error while testing it on ezid-stg. Can you take a look.
Traceback (most recent call last):
File "/apps/ezid/ezid-ops-scripts/scripts/verify_ezid_after_patching.py", line 562, in <module>
main()
File "/apps/ezid/ezid-ops-scripts/scripts/verify_ezid_after_patching.py", line 559, in main
vap.check_resolver()
File "/apps/ezid/ezid-ops-scripts/scripts/verify_ezid_after_patching.py", line 500, in check_resolver
status = self._get_status(f"{base_url}/{id}", allow_redirects=False)
^^^^^^^^
NameError: name 'base_url' is not defined
Thank you
Jing
Ok, I updated this again. It was weird since there were a few code changes missing which I thought had gotten committed.
|
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.
@sfisher Hi Scott,
Since we have removed verifying background job status from the script, now we can run the test scripts from other locations outside of the EZID instances. How about we add a readme file to explain how to setup an environment to run the scripts.
Jing
Ok. Well, the download is using a background job, which is why I wondered about it. For dev/local then it needs to be started for the tests to succeed. What are your thoughts on this. Should the the download be automatically skipped on dev/localhost? Or should we start it up manually every time? Also, where do you prefer these instructions? In this repo or in the ezid-docs-internal or both? |
For the instructions, we may want a |
We can remove |
Regarding on |
I added a file It also seems like we might move toward either a container or github actions for running some tests where we could trigger as either part of a deploy or as a manual test from gh actions. In that case it could be a different model than running locally. |
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.
The tests are comprehensive and the instructions look great!
Thank you Scott!
Jing
This adds the tests mentioned in CDLUC3/ezid#853 .
I believe this now covers a fairly full set of functionality for the EZID API.
I also did a bunch of refactoring to make this object oriented since there were a lot of method taking parameters like base_url, user, password which we can just set as member variables on a class and use them from there without having to pass them all the time.
I removed the numbering since it's hard to maintain if we add or remove tests and adds complications I don't think are necessary. I just made things like heading and then indented underneath so it's clear how tests are related.
The command to run is the same as before but just asks many more tests.