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

Use pytest instead of unittest #390

Merged
merged 9 commits into from Jul 16, 2020

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Jul 4, 2020

Closes #270

This PR moves the tests completely over to the pytest way of doing things, instead of unittest.
There is only a single place left that still uses unittest, specifically the unittest.mock.patch feature.
This may be converted to pytest's monkeypatch as well, but for now I didn't see the reason.

The conversion makes much of the testing redundancy superfluous, due to the usage of the pytest fixtures and parametrizations.
There is potentially still room for further simplifications, when testing the entry endpoints, however then the file structure and concept division may fail (if, e.g., similar tests for /references and /structures are combined using parametrization, it may be more obfuscating than actuallly helpful and clear).

The client_factory() is technically not necessary. However, since I found it useful in aiida-optimade for a middleware test that redirects the documentation URLs between the versioned URLs, I have left it in here, if similar tests involving the versioned base URLs become relevant.

@CasperWA CasperWA requested review from ml-evs and shyamd July 4, 2020 04:50
@codecov
Copy link

codecov bot commented Jul 4, 2020

Codecov Report

Merging #390 into master will increase coverage by 0.73%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   89.62%   90.36%   +0.73%     
==========================================
  Files          55       55              
  Lines        2459     2459              
==========================================
+ Hits         2204     2222      +18     
+ Misses        255      237      -18     
Flag Coverage Δ
#unittests 90.36% <100.00%> (+0.73%) ⬆️
Impacted Files Coverage Δ
optimade/server/entry_collections/mongo.py 95.91% <100.00%> (+9.18%) ⬆️
optimade/server/routers/utils.py 90.34% <0.00%> (-0.69%) ⬇️
optimade/server/main.py 95.74% <0.00%> (+10.63%) ⬆️
optimade/server/main_index.py 95.74% <0.00%> (+10.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec825ef...89db721. Read the comment docs.

@CasperWA CasperWA mentioned this pull request Jul 4, 2020
10 tasks
ml-evs
ml-evs previously approved these changes Jul 9, 2020
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Looks great @CasperWA, just a couple of minor things that don't stop me accepting

tests/server/conftest.py Outdated Show resolved Hide resolved
tests/server/conftest.py Show resolved Hide resolved
tests/server/utils.py Outdated Show resolved Hide resolved
CasperWA and others added 6 commits July 15, 2020 17:30
Update handling of the `sort` parameter, casting the requested fields to
the known aliased version.
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Minor stuff, looks like the checks aren't running atm anyway... happy to accept shortly

tests/server/utils.py Show resolved Hide resolved
tests/server/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
ml-evs
ml-evs previously approved these changes Jul 15, 2020
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Great work as always, cheers @CasperWA

@CasperWA
Copy link
Member Author

I needed to try and push something else, in order to try and get the pre-commit test running correctly.
So I have updated all default installations of Python 3.7 in the CI to Python 3.8.
Feel free to re-approve when tests succeed @ml-evs :)

@CasperWA
Copy link
Member Author

CasperWA commented Jul 16, 2020

Hmm. I'm not touching any files in this PR that would influence the OpenAPI generation. So I am very confused by the sudden failure of the OpenAPI diff check in pre-commit CI.

Also, I cannot reproduce the error locally.

@ml-evs
Copy link
Member

ml-evs commented Jul 16, 2020

Also, I cannot reproduce the error locally.

I can reproduce it in a fresh Python 3.8 env, but can't in the 3.7 env I use for development. Will see if there's a diff between the packages getting installed.

@ml-evs
Copy link
Member

ml-evs commented Jul 16, 2020

It's the upgrade to pydantic 1.6.1 that's causing the diff, now to figure out why pre-commit is using that version...

@ml-evs
Copy link
Member

ml-evs commented Jul 16, 2020

Here's the change that's causing it too, from https://github.com/samuelcolvin/pydantic/blob/master/HISTORY.md

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

👍

@CasperWA
Copy link
Member Author

Here's the change that's causing it too, from https://github.com/samuelcolvin/pydantic/blob/master/HISTORY.md

This change is actually really good. It cleans up the OpenAPI schema 👍
But let's wait until dependabot complains to treat this - or force an update from dependabot.
In any case, your addition of the requirements*.txt files is appropriate.

@CasperWA CasperWA merged commit ec207fa into Materials-Consortia:master Jul 16, 2020
@CasperWA CasperWA deleted the use_pytest branch July 16, 2020 10:10
@ml-evs
Copy link
Member

ml-evs commented Jul 16, 2020

Here's the change that's causing it too, from https://github.com/samuelcolvin/pydantic/blob/master/HISTORY.md

This change is actually really good. It cleans up the OpenAPI schema +1
But let's wait until dependabot complains to treat this - or force an update from dependabot.
In any case, your addition of the requirements*.txt files is appropriate.

Yeah it looks good, we should remember to upgrade pydantic before we next merge back into the spec repo (if that is before dependabot triggers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move tests to pytest system from unittest
2 participants