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

Adjusted check_response so it can also handle single entries. #1130

Conversation

JPBergsma
Copy link
Contributor

In #1125 I noticed that the check_response function did not work for single entry endpoints.
This code should fix that issue.

closes #1125

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #1130 (c4c8721) into master (6133cf1) will not change coverage.
The diff coverage is n/a.

❗ Current head c4c8721 differs from pull request most recent head 8a06903. Consider uploading reports for the commit 8a06903 to get more accurate results

@@           Coverage Diff           @@
##           master    #1130   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files          68       68           
  Lines        3889     3889           
=======================================
  Hits         3593     3593           
  Misses        296      296           
Flag Coverage Δ
project 92.38% <ø> (ø)
validator 92.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 f6309e6...8a06903. Read the comment docs.

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 changes requested below.

Do you plan on using this functionality somewhere? I see the other fixtures in the query params conftest.py (like checking for required response fields) will also only work for the multi-entry endpoints. Should they be updated too?

tests/server/conftest.py Outdated Show resolved Hide resolved
tests/server/routers/test_structures.py Outdated Show resolved Hide resolved
@JPBergsma
Copy link
Contributor Author

I tried to use a test like this to find out what was going wrong with the single entry endpoint for the trajectories.
I could however also have formulated the test like the test classes in test_structures.py, so it is not so important.

@JPBergsma
Copy link
Contributor Author

@ml-evs I have processed your comments.
Could you either approve this pull request or state that you do not think it is worth adding.
That way this PR can be closed.

@ml-evs
Copy link
Member

ml-evs commented May 9, 2022

Sorry @JPBergsma, this one got lost. If you think it is useful we can merge it; I'll rebase -> accept -> merge now if all the tests still pass.

JPBergsma and others added 2 commits May 9, 2022 11:25
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@ml-evs ml-evs force-pushed the JPBergsma_check_response_single_entry branch from c4c8721 to 8a06903 Compare May 9, 2022 10:25
@ml-evs ml-evs enabled auto-merge (squash) May 9, 2022 10:28
@ml-evs ml-evs merged commit 610ed07 into Materials-Consortia:master May 9, 2022
@JPBergsma JPBergsma deleted the JPBergsma_check_response_single_entry branch May 9, 2022 11:02
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.

Test on Queries on single structures fail with the check_response function.
2 participants