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

Check pagination links->next with validator #266

Merged
merged 7 commits into from May 6, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented May 5, 2020

Closes #265.

@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch 2 times, most recently from 7e32ded to f5e21a3 Compare May 5, 2020 14:35
@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #266 into master will decrease coverage by 0.16%.
The diff coverage is 70.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   89.32%   89.15%   -0.17%     
==========================================
  Files          54       54              
  Lines        2220     2241      +21     
==========================================
+ Hits         1983     1998      +15     
- Misses        237      243       +6     
Flag Coverage Δ
#unittests 89.15% <70.37%> (-0.17%) ⬇️
Impacted Files Coverage Δ
optimade/validator/validator.py 68.08% <70.37%> (+0.26%) ⬆️

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 2eddcb7...3452cb4. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch 3 times, most recently from ce647d6 to 48490b0 Compare May 5, 2020 15:40
@ml-evs
Copy link
Member Author

ml-evs commented May 5, 2020

Having lots of fun getting the docker server under CI to return the right base URL in pagination links...

@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from 48490b0 to fc14aad Compare May 5, 2020 15:57
@ml-evs
Copy link
Member Author

ml-evs commented May 5, 2020

Done now, with a bit of a CI-specific hack to the config file.

@ml-evs ml-evs requested a review from CasperWA May 5, 2020 16:04
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Great! Thanks @ml-evs !

We're slowly turning our tests over to the validator, as it should be.

I have some comments and suggestions for improvements, hope they help :)

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from fc14aad to 5599411 Compare May 5, 2020 18:27
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from 5599411 to 185dc70 Compare May 5, 2020 18:31
@ml-evs
Copy link
Member Author

ml-evs commented May 5, 2020

Thanks for the review @CasperWA, think I've implemented the spirit of your suggestions if not the exact letter of them :)

@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from fffc4b1 to a46a09c Compare May 5, 2020 18:49
@ml-evs ml-evs requested a review from CasperWA May 5, 2020 19:12
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from a46a09c to abbcf54 Compare May 6, 2020 10:12
.ci/docker_config.json Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Show resolved Hide resolved
optimade/validator/validator.py Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from abbcf54 to 1e6ea44 Compare May 6, 2020 10:21
ml-evs and others added 2 commits May 6, 2020 11:22
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from 1e6ea44 to cd0beb2 Compare May 6, 2020 10:36
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/validate_pagination branch from cd0beb2 to 8d28119 Compare May 6, 2020 10:37
@CasperWA
Copy link
Member

CasperWA commented May 6, 2020

It seems the validator is not getting the version included in it's calls to the Dockerized server.

@ml-evs
Copy link
Member Author

ml-evs commented May 6, 2020

It seems the validator is not getting the version included in it's calls to the Dockerized server.

Ugh, looks like urllib.parse.urljoin("http://host.com/v0", "/structures?filter=a=1") returns http://host.com/structures?filter=a=1. I'll go back and undo those changes...

@ml-evs
Copy link
Member Author

ml-evs commented May 6, 2020

Think we're finally there, don't really want to make any other tweaks to the validator in this PR as it probably need a full refactoring pass to do it properly.

@ml-evs ml-evs requested a review from CasperWA May 6, 2020 11:40
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

It sure is looking great now, thanks @ml-evs!
I have only a single comment (with included change suggestion) left.
I will approve now, if you don't feel the change is needed, otherwise, please apply and I'll re-approve for you to squash+merge.

@@ -113,10 +120,12 @@ def get(self, request: str):
the `MAX_RETRIES` attempts.

"""
if request:
self.last_request = f"{self.base_url}{request}".replace("\n", "")
if urllib.parse.urlparse(request, allow_fragments=True).scheme:
Copy link
Member

Choose a reason for hiding this comment

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

Do you still want to make sure the scheme is part of ("http", "https") or do we not care? If yes, here's a quick and dirty suggestion:

Suggested change
if urllib.parse.urlparse(request, allow_fragments=True).scheme:
request_scheme = urllib.parse.urlparse(request, allow_fragments=True).scheme
if request_scheme and request_scheme in ("http", "https"):

Otherwise, fine 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to leave it as is, on the off-chance that an implementation provides a different scheme (presumably accidentally) then it will be obvious from the error spat out. Your suggested change would lead to the next request being e.g. http://base_url.com/ssh://base_url.com?page_offset=5, so I think it's clearer as is.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Then the docstring should probably have been updated, but fine.
Again, I just want to be really nitpicky with everything in the validator since (in my opinion) it should be usable for every and any implementation. I.e., it should be really specific in what it wants, while at the same time being very broad in scope.. not easy :)

@ml-evs ml-evs merged commit b5cc923 into master May 6, 2020
@ml-evs ml-evs mentioned this pull request May 6, 2020
@CasperWA CasperWA deleted the ml-evs/validate_pagination branch May 6, 2020 14:17
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.

Validator does not check that pagination links work
2 participants