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

Export test fixes #2887

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Export test fixes #2887

merged 9 commits into from
Nov 28, 2023

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Nov 24, 2023

Was looking at export tests for #2885 and spotted a few changes to make!

Refs #2885

@A5rocks A5rocks mentioned this pull request Nov 24, 2023
17 tasks
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Merging #2887 (ca46414) into master (9c496fa) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2887      +/-   ##
==========================================
- Coverage   99.54%   99.54%   -0.01%     
==========================================
  Files         115      115              
  Lines       17660    17657       -3     
  Branches     3165     3164       -1     
==========================================
- Hits        17580    17577       -3     
  Misses         52       52              
  Partials       28       28              
Files Coverage Δ
src/trio/__init__.py 100.00% <100.00%> (ø)
src/trio/_tests/test_exports.py 98.82% <100.00%> (-0.03%) ⬇️
src/trio/socket.py 100.00% <100.00%> (ø)

Copy link
Contributor

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Comment on lines +160 to +161
if sys.implementation.name != "cpython":
pytest.skip("jedi does not support pypy")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if sys.implementation.name != "cpython":
pytest.skip("jedi does not support pypy")
if sys.implementation.name != "cpython": # pragma: no branch
pytest.skip("jedi does not support pypy")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels weird though, like I don't think codecov should be complaining about this :(

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, this is the issue where we have double codecov runs on some PRs - and the inline errors are taken from the "incorrect" codecov run, I've seen this a couple times.
That's also the same as #2887 (comment) not getting updated.

I was gonna try and link them .. when I noticed that one set links to https://app.codecov.io/gh/python-trio/trio/commit/c74f51bf891c940d4ec7ed5859e55ab91a2a1809 and the other links to https://github.com/python-trio/trio/pull/2887/checks?check_run_id=19006357499 in the "details" link in the summary. Which is maybe a good hint to what's causing it?

We need some more messing around with codecov 🙃, see also #2689

@@ -67,13 +67,19 @@
ntohs as ntohs,
)

# TODO: update python docs to reflect this
Copy link
Member

Choose a reason for hiding this comment

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

mostly just this that's a blocker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I made a typeshed PR thinking "oh well the stdlib docs are the spec, so PyPy is deviating from it and should not have the docs update for it" but then now I realize sys.implementation.name isn't a guard typeshed allows :(

And it looks like these functions just never exist on PyPy. Lol.

@CoolCat467
Copy link
Contributor

With ca46414 eliminating jakkdl's blocker, I am going to merge this.

@CoolCat467 CoolCat467 merged commit 59731c3 into python-trio:master Nov 28, 2023
29 checks passed
@A5rocks A5rocks deleted the export-test-fixes branch November 28, 2023 04:41
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.

None yet

4 participants