test: make GC check resilient to broken weakrefs#1099
Merged
tswast merged 4 commits intogoogleapis:mainfrom Jan 18, 2022
Merged
test: make GC check resilient to broken weakrefs#1099tswast merged 4 commits intogoogleapis:mainfrom
tswast merged 4 commits intogoogleapis:mainfrom
Conversation
In some python deployments (I believe macOS is one), weakref objects aren't guaranteed to exist across gc collections. The proposed check pattern is also used in tensorflow [0] which ran into similar issues a few years ago. [0]: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/framework/errors_test.py#L33
tswast
requested changes
Jan 6, 2022
Contributor
tswast
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I hadn't noticed any flakes when running the test suite locally on my Mac, but could just be a rare thing.
Contributor
Author
|
Any idea what the conventional commit error is? |
Contributor
This is the output, accessible by clicking the "Details" link next to the failed check (is it displayed for you?): BTW, I changed the title of the PR to fix that (as an alternative to amending the commit message itself), added the most sensible prefix ( |
plamut
reviewed
Jan 10, 2022
Contributor
There was a problem hiding this comment.
LGTM, just have a clarification question, thanks!
Edit: Please just fix the indentation (4 spaces should be used) to conform to this project's style guide, which is why one of the checks failed:
nox > flake8 tests
tests/unit/test_dbapi_connection.py:224:11: E111 indentation is not a multiple of 4
tests/unit/test_dbapi_connection.py:226:15: E111 indentation is not a multiple of 4
tests/unit/test_dbapi_connection.py:227:11: E111 indentation is not a multiple of 4
nox > Command flake8 tests failed with exit code 1
nox > Session lint failed.
plamut
approved these changes
Jan 17, 2022
Contributor
|
@tswast I don't have write access anymore, please re-run the tests and merge the fix, thanks! |
tswast
approved these changes
Jan 18, 2022
abdelmegahedgoogle
pushed a commit
to abdelmegahedgoogle/python-bigquery
that referenced
this pull request
Apr 17, 2023
* fix: Make gc check resilient to broken weakrefs In some python deployments (I believe macOS is one), weakref objects aren't guaranteed to exist across gc collections. The proposed check pattern is also used in tensorflow [0] which ran into similar issues a few years ago. [0]: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/framework/errors_test.py#L33 * fix: add pragma for coverage * Fix whitespace
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In some python deployments (I believe macOS is one), weakref objects aren't guaranteed to exist across gc collections. The proposed check pattern is also used in tensorflow (link) which ran into similar issues a few years ago.