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

test: use file in skhep-testdata for issue #121 #973

Merged
merged 7 commits into from
Oct 5, 2023
Merged

Conversation

lobis
Copy link
Collaborator

@lobis lobis commented Oct 4, 2023

No description provided.

@lobis lobis changed the title Use file in skhep-testdata for issue #121 test: use file in skhep-testdata for issue #121 Oct 4, 2023
@lobis lobis marked this pull request as ready for review October 4, 2023 21:39
@lobis lobis requested a review from jpivarski October 4, 2023 21:39
@lobis lobis added the tests More or better tests are needed label Oct 4, 2023
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

See below!

tests/test_0088-read-with-http.py Outdated Show resolved Hide resolved
tests/test_0692_fsspec.py Outdated Show resolved Hide resolved
tests/test_0692_fsspec.py Outdated Show resolved Hide resolved
tests/test_0692_fsspec.py Outdated Show resolved Hide resolved
@lobis lobis requested review from jpivarski and nsmith- October 5, 2023 14:11
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Is the github:// test using api.github.com with its rate limit? (This is continued from a conversation on Slack.)

If it is, let's just drop the github:// test. We can assume that fsspec does its job correctly (managing exotic backends like the GitHub one) and just verify that our connection to fsspec is doing what it's supposed to. We should continue testing local files, HTTP, XRootD, and maybe S3 because Uproot users are known to use them, but I'd say that the GitHub one is just "nice to have."

Other than that, I think this PR is ready to merge.

Copy link
Member

@nsmith- nsmith- left a comment

Choose a reason for hiding this comment

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

If the github URL has a rate limit policy that may cause tests to fail then yeah it makes sense to drop it. Otherwise, looks good!

@lobis
Copy link
Collaborator Author

lobis commented Oct 5, 2023

Is the github:// test using api.github.com with its rate limit? (This is continued from a conversation on Slack.)

If it is, let's just drop the github:// test. We can assume that fsspec does its job correctly (managing exotic backends like the GitHub one) and just verify that our connection to fsspec is doing what it's supposed to. We should continue testing local files, HTTP, XRootD, and maybe S3 because Uproot users are known to use them, but I'd say that the GitHub one is just "nice to have."

Other than that, I think this PR is ready to merge.

I think I will keep the pytest skip instead of just removing the test, I think it's nice to have an example of some of the exotic fsspec sources. I think there is some value in testing it explicitly though, for example the github schema url has two : in it which made the old "split path and object" helper method fail. I guess we could explicitly test the helper method instead through.

@lobis lobis merged commit 576fbdb into main Oct 5, 2023
15 checks passed
@lobis lobis deleted the uproot-121-file branch October 5, 2023 21:04
lobis added a commit that referenced this pull request Oct 5, 2023
* origin/main:
  test: use file in skhep-testdata for issue #121 (#973)
@jpivarski
Copy link
Member

Okay. If we're ever working on something unrelated and see failures in this test, we'll unceremoniously drop it. You'll notice that some of the @pytest.mark.skip reasons are "such-and-such a server is flaky."

lobis added a commit that referenced this pull request Oct 12, 2023
* single argument for handlers

* style: pre-commit fixes

* fix missing object source

* add deprecation warnings for handler options

* style: pre-commit fixes

* use handler instead of *_handler

* use file in skhep testdata

* remove hyphen

* use tag instead of branch (#973 (comment))

* remove network pytest mark (#973 (comment))

* use proper http links instead of local paths

* use fsspec to split url

* add comment

* fix bad strip

* working github test

* add test for path split object

* Update src/uproot/_util.py

Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>

* use urllib instead of fsspec for url parsing

* move tests to dedicated file

* do not shadow `object`

* add more test cases

* fsspec source as default

* fix test

* add xrootd handler default

* correctly strip obj

* revert merge

* Revert "revert merge"

This reverts commit 60300d6.

* Revert "Revert "revert merge""

This reverts commit 2884e02.

* update docstrings

* update docstrings

* direct the user to the handler option in docs

* explain order of handler options

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests More or better tests are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants