chore: generate grafeas - #17307
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the minimum grpcio dependency version to 1.59.0, adjusts Nox configuration to dynamically resolve constraints files, and refactors unit tests to parameterize request types. Feedback on these changes highlights a potential issue in noxfile.py where dynamic path resolution using ALL_PYTHON[0] might point to a non-existent file if the corresponding constraints file is missing. Additionally, in test_grafeas.py, passing instantiated objects like {} in @pytest.mark.parametrize introduces shared mutable state across tests and makes the parameter name request_type misleading; it is recommended to pass types and instantiate them within the tests instead.
| if (CURRENT_DIRECTORY / "testing").exists(): | ||
| LOWER_BOUND_CONSTRAINTS_FILE = ( | ||
| CURRENT_DIRECTORY / "testing" / f"constraints-{ALL_PYTHON[0]}.txt" | ||
| ) | ||
| else: | ||
| LOWER_BOUND_CONSTRAINTS_FILE = CURRENT_DIRECTORY / "constraints.txt" |
There was a problem hiding this comment.
If ALL_PYTHON[0] (the minimum Python version) does not match the version in the constraints file name (e.g., if ALL_PYTHON[0] is "3.7" but only constraints-3.10.txt exists), this dynamic path resolution will point to a non-existent file and cause Nox sessions to fail.
Consider adding a fallback check to ensure the file exists, or dynamically locate the constraints file using a glob pattern.
| @pytest.mark.parametrize( | ||
| "request_type", | ||
| [ | ||
| grafeas.GetOccurrenceRequest, | ||
| dict, | ||
| grafeas.GetOccurrenceRequest(), | ||
| {}, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
There are two issues with this parameterization:
- Shared Mutable State: Passing
{}as a parameter in@pytest.mark.parametrizemeans the same dictionary instance is shared across all test runs. If any test or underlying client code mutates the request, it will pollute the state for subsequent tests, potentially causing flaky tests. - Misleading Naming: The parameter is named
request_type, but it now receives an instance (e.g.,GetOccurrenceRequest()or{}) instead of a type/class.
It is safer and cleaner to pass the classes/types (or callables) and instantiate them inside the test (e.g., request = request_type()), which guarantees a fresh, independent instance for every test execution.
| @pytest.mark.parametrize( | |
| "request_type", | |
| [ | |
| grafeas.GetOccurrenceRequest, | |
| dict, | |
| grafeas.GetOccurrenceRequest(), | |
| {}, | |
| ], | |
| ) | |
| @pytest.mark.parametrize( | |
| "request_type", | |
| [ | |
| grafeas.GetOccurrenceRequest, | |
| dict, | |
| ], | |
| ) |
Towards #17305
See follow up issue #17311