Fix CI errors and tear down socket errors #770
Conversation
julianz-
force-pushed
the
fix-builtin-wrap
branch
from
506a4df to
56ac887
Compare
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 |
julianz-
force-pushed
the
fix-builtin-wrap
branch
18 times, most recently
from
b55e4c6 to
775d221
Compare
- 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-
force-pushed
the
fix-builtin-wrap
branch
from
775d221 to
f9bbfed
Compare
- 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
|
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. |
julianz-
force-pushed
the
fix-builtin-wrap
branch
from
6b6647b to
8e90a1c
Compare
…into fix-builtin-wrap
julianz-
force-pushed
the
fix-builtin-wrap
branch
from
8e90a1c to
70bfa38
Compare
|
@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.
I extracted this into main. 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.
I need to work up a fix I can submit to schemastore for this, but I did... sort of figure it out.
The definition of a pull_request event mixes explicit properties with a $ref usage (the target of the JSON Schema $ref is also named "ref", which is a little quirky but fine). When $ref loading is mixed 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.
I did end up getting a 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 which check-jsonschema can't catch until this is fixed.
|
|
||
| # 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.
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.
This is already pinned to a higher version in a proper place.
| PYTHONDONTWRITEBYTECODE=x | ||
| WEBTEST_INTERACTIVE=false | ||
|
|
||
|
|
| raise reraised_connection_drop_exc_cls( | ||
| *generic_tls_error.args, | ||
| ) from generic_tls_error | ||
| except OSError as tcp_connection_drop_error: |
There was a problem hiding this comment.
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.
| ) from tls_connection_drop_error | ||
| except ssl.SSLError as generic_tls_error: | ||
| peer_speaks_plain_http_over_https = ( | ||
| generic_tls_error.errno == ssl.SSL_ERROR_SSL |
There was a problem hiding this comment.
What other error codes can this case have?
3 participants
This PR deal with numerous errors that arise on submitting even the smallest changes to CI.
This change is