Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

test: make GC check resilient to broken weakrefs - #1099

Merged
tswast merged 4 commits into
googleapis:mainfrom
msuozzo:patch-1
Jan 18, 2022
Merged

test: make GC check resilient to broken weakrefs#1099
tswast merged 4 commits into
googleapis:mainfrom
msuozzo:patch-1

Conversation

@msuozzo

@msuozzomsuozzo commented Jan 6, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@msuozzo
msuozzo requested review from a team and shollymanJanuary 6, 2022 01:27
@product-auto-labelproduct-auto-labelBot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jan 6, 2022
@msuozzomsuozzo changed the title Make gc check resilient to broken weakrefsfix: Make gc check resilient to broken weakrefsJan 6, 2022
@msuozzomsuozzo changed the title fix: Make gc check resilient to broken weakrefsMake gc check resilient to broken weakrefsJan 6, 2022
@msuozzo
fix: Make gc check resilient to broken weakrefs
2b7022f
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
@shollyman
shollyman requested a review from tswastJanuary 6, 2022 03:02
tswast
tswast suggested changes Jan 6, 2022
View reviewed changes

@tswasttswast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadtests/unit/test_dbapi_connection.py Outdated
@msuozzo

msuozzo commented Jan 7, 2022

Copy link
Copy Markdown
ContributorAuthor

Any idea what the conventional commit error is?

@plamut

plamut commented Jan 10, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Any idea what the conventional commit error is?

This is the output, accessible by clicking the "Details" link next to the failed check (is it displayed for you?):

Linting errors for "make gc check resilient to broken weakrefs"
- subject may not be empty
- type may not be empty

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 (test:).

@plamutplamut changed the title Make gc check resilient to broken weakrefstest: make GC check resilient to broken weakrefsJan 10, 2022
plamut
plamut reviewed Jan 10, 2022
View reviewed changes

@plamutplamut left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadtests/unit/test_dbapi_connection.py
@plamutplamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2022
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2022
@plamut

plamut commented Jan 17, 2022

Copy link
Copy Markdown
Contributor

@tswast I don't have write access anymore, please re-run the tests and merge the fix, thanks!

@tswasttswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2022
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2022
@tswast
tswast merged commit f54d81f into googleapis:mainJan 18, 2022
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
@msuozzo@abdelmegahedgoogle
test: make GC check resilient to broken weakrefs (googleapis#1099)
302f2d6
* 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
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@shollymanshollymanAwaiting requested review from shollyman
2 more reviewers
@tswasttswasttswast approved these changes
@plamutplamutplamut approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@msuozzo@plamut@tswast@yoshi-kokoro