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

[TST] Refactor tests #269

Merged
merged 4 commits into from
Jan 29, 2024
Merged

[TST] Refactor tests #269

merged 4 commits into from
Jan 29, 2024

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Jan 26, 2024

Changes proposed in this pull request:

  • Basic test added for structure of cohort query response when RETURN_AGG=true
  • Turned mocked get function used across query tests into a few different fixtures, with variations based on the type of value/object to be returned by the mocked function
    • Specific return objects for the mocked function specified in test definitions via indirect parametrization
    • mock_successful_get (which depends on another fixture for the return object) kept a separate function to avoid overly complicated parametrization when writing tests (i.e., you cannot easily pass one fixture as a parameter to another fixture)

Checklist

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@alyssadai alyssadai added the pr-tests Add or improve existing tests label Jan 26, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jan 26, 2024

Pull Request Test Coverage Report for Build 7702731794

  • 0 of 52 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 99.784%

Totals Coverage Status
Change from base Build 7648049836: 0.005%
Covered Lines: 925
Relevant Lines: 927

💛 - Coveralls

@alyssadai alyssadai marked this pull request as ready for review January 26, 2024 23:59
@surchs surchs self-requested a review January 29, 2024 15:30
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool PR @alyssadai, thanks for figuring all of this out.

Looking over the double parameterization here is impressive, but also a bit confusing:

  1. when I read the parameterization I wonder: what is parameterized? There's only one parameter
  2. when I read the test itself where I am using the mock I wonder: what does this mock actually do?

I think we could probably live with that, but my sense is that pytest offers a simpler way to achieve the same outcome: https://docs.pytest.org/en/6.2.x/fixture.html#using-markers-to-pass-data-to-fixtures

Maybe you could take a look at that section (and the "Factories as fixtures" just after it) and see if you agree.

Otherwise: 🧑‍🍳

@alyssadai alyssadai merged commit 684b48e into main Jan 29, 2024
6 checks passed
@alyssadai alyssadai deleted the refactor-tests branch January 29, 2024 21:44
@surchs
Copy link
Contributor

surchs commented Apr 11, 2024

🚀 PR was released in v0.2.0 🚀

@surchs surchs added the released This issue/pull request has been released. label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-tests Add or improve existing tests released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor mock get method for testing to accept return object Write a test for behaviour of RETURN_AGG option
3 participants