Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork99
Fix CI errors and tear down socket errors#770
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
506a4df to56ac887Comparecodecovbot commentedSep 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #770 +/- ##==========================================+ Coverage 79.31% 79.44% +0.12%========================================== Files 29 29 Lines 4197 4277 +80 Branches 538 545 +7 ==========================================+ Hits 3329 3398 +69- Misses 726 733 +7- Partials 142 146 +4 |
b55e4c6 to775d221Compare- Fixed socket teardown errors that were thrown during connection cleanup- Added missing __all__ declarations to server.pyi and threadpool.pyi- Removed incorrect WIN_SOCKET_NOT_OPEN definition from makefile.pyi- Resolved stubtest failures across multiple modules
775d221 tof9bbfedCompare- Fixed socket teardown errors that were thrown during connection cleanup- Added missing __all__ declarations to server.pyi and threadpool.pyi- Removed incorrect WIN_SOCKET_NOT_OPEN definition from makefile.pyi- Resolved stubtest failures across multiple modules
julianz- commentedSep 23, 2025
I put this PR together because I couldn't get#764 to pass the CI for reasons that seemed unrelated to the code changes there. |
6b6647b to8e90a1cCompare…into fix-builtin-wrap
8e90a1c to70bfa38Comparewebknjaz commentedSep 27, 2025
@julianz- could you send these as multiple atomic standalone PRs so it's easier to review and understand in general? |
| -pre-commit-ci-update-config# pre-commit.ci always creates a PR | ||
| pull_request: | ||
| ignore-paths:# changes to the cron workflow are triggered through it | ||
| paths-ignore:# changes to the cron workflow are triggered through it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I extracted this intomain. I have this typo across a bunch of repos, actually. Good catch!
@sirosen any insight into why jsonschema didn't catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I need to work up a fix I can submit to schemastore for this, but I did... sort of figure it out.
Thedefinition of apull_request event mixes explicit properties with a$ref usage (the target of the JSON Schema$ref is alsonamed"ref", which is a little quirky but fine). When$ref loading ismixed with an explicit schema, as in this case, I don't have any clear expectation about what should happen. It might be covered by the spec, but it's definitely going to trip up implementations.
The definition of"ref" looks fine at a glance:
https://github.com/SchemaStore/schemastore/blame/71c836f7a50aa0f796c990c433307dc7b87e300e/src/schemas/json/github-workflow.json#L340-L386
But I notice that it doesn't set"additionalProperties": false, and I think that's the issue. If I remove that$ref usage in a copy of the schema, I get the appropriate error.
It looks like this is the only usage site for#/definitions/ref, so I think I'll just inline it and add a test which demonstrates the issue. JSON Schema is always a headtrip! 😵💫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did end up gettinga PR posted!
It ate a lot of my free time for FOSS work this evening, but I think it's worth it. There's a pretty large class of mistakes whichcheck-jsonschema can't catch until this is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@sirosen nice, thanks!
| # A mapping of markers to their descriptions allowed in strict mode: | ||
| markers = | ||
| flaky: mark a test as flaky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This shouldn't be needed as it's coming from a plugin.
| [testenv] | ||
| deps = | ||
| -rrequirements{/}tests.in | ||
| pytest-xdist>=3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is already pinned to a higher version in a proper place.
| PYTHONDONTWRITEBYTECODE=x | ||
| WEBTEST_INTERACTIVE=false | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
unrelated
| raisereraised_connection_drop_exc_cls( | ||
| *generic_tls_error.args, | ||
| )fromgeneric_tls_error | ||
| exceptOSErrorastcp_connection_drop_error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand why you felt the need to move this into another except-block. That makes it more complicated to follow. And AFAICS the lotig is about the same just more branchy. I was keeping it separate so that it's simple.
| )fromtls_connection_drop_error | ||
| exceptssl.SSLErrorasgeneric_tls_error: | ||
| peer_speaks_plain_http_over_https= ( | ||
| generic_tls_error.errno==ssl.SSL_ERROR_SSL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What other error codes can this case have?
Uh oh!
There was an error while loading.Please reload this page.
This PR deal with numerous errors that arise on submitting even the smallest changes to CI.
This change is