-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor for builder changes and Individual Datasets #7
Refactor for builder changes and Individual Datasets #7
Conversation
… up from RO-Crate group
* add gpg to PR workflow * add cli tests
pyproject.toml
Outdated
ro-crate-py = {git = "https://github.com/UoA-eResearch/ro-crate-py.git", branch = "encrypted-metadata"} | ||
openpyxl = "^3.1.5" | ||
click = "^8.1.7" | ||
faker = "^27.4.0" |
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.
Do Faker, FactoryBoy and Hypothesis want to sit in general dependencies or should these be dev dependencies? I.e. do we expect our users to run the tests?
Just a thought
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.
Moved them to a test group, I don't think I even ended up needing hypothesis and factory boy so will remove those
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.
Should this be in a separate .env file? or in the conftest.py?
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.
There is a value in the ENV file that updates this with a new pubkey this one is just hard-coded in by default.
I should definitely update the email to a dummy value.
tests/conftest.py
Outdated
|
||
|
||
def fake_upi(faker: Faker) -> str: | ||
return faker.bothify(text="???###", letters="abcdefghijklmnopqrstuvwxyz") |
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.
Is this missing a character? Should be ????### IIRC :)
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.
I'm guessing this is included by mistake :)
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.
yes removed :)
|
||
|
||
@responses.activate | ||
def test_mytardis_rest_agent_get( |
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.
Not sure what this test is aiming to do. The response has been mocked so asserting that the status is 200 is not useful here. It does demonstrate that a get request has been made :)
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.
Basically all that was for, that a request happens at all. Added a few tests for bad requests to make it more complete.
tests/test_apis.py
Outdated
|
||
response = mt_rest_agent.no_auth_request("GET", url) | ||
do_not_use_auth.assert_not_called() | ||
assert isinstance(response.json(), dict) |
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.
These three tests are mocked so asserting them is surplus to requirements I think.
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.
reduced just down to checking the auth is not used
Two major changes in this PR.
Refactored based on builder changes
Print Lab Extractor now Produces individual dataset Crates
Added an ICD-11 lookup API for medical condition data
Added tests for all print lab data extraction and generic metadata extraction