Skip to content

test(spanner): revert noxfile changes and disable AFE server timing in conftest - #17980

Open
sakthivelmanii wants to merge 2 commits into
mainfrom
fix-spanner-noxfile-regeneration
Open

test(spanner): revert noxfile changes and disable AFE server timing in conftest#17980
sakthivelmanii wants to merge 2 commits into
mainfrom
fix-spanner-noxfile-regeneration

Conversation

@sakthivelmanii

Copy link
Copy Markdown
Contributor

Reverts the edits to packages/google-cloud-spanner/noxfile.py from PR #17970 to restore compliance with owlbot/librarian autogeneration and fix issue #17974.

Disables AFE server timing in unit tests directly inside tests/unit/conftest.py by setting SPANNER_DISABLE_AFE_SERVER_TIMING="true" and _helpers.ENABLE_AFE_SERVER_TIMING = False. This ensures both standard unit tests and prerelease-deps unit tests pass cleanly without modifying autogenerated files or existing unit test assertions.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

…n conftest

Reverts the edits to packages/google-cloud-spanner/noxfile.py from PR #17970 to restore compliance with owlbot/librarian autogeneration and fix issue #17974.

Disables AFE server timing in unit tests directly inside tests/unit/conftest.py by setting SPANNER_DISABLE_AFE_SERVER_TIMING="true" and _helpers.ENABLE_AFE_SERVER_TIMING = False. This ensures both standard unit tests and prerelease-deps unit tests pass cleanly without modifying autogenerated files or existing unit test assertions.
@sakthivelmanii
sakthivelmanii requested a review from a team as a code owner August 3, 2026 18:45
@sakthivelmanii
sakthivelmanii requested a review from parthea August 3, 2026 18:45

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request centralizes the disabling of AFE server timing for unit tests by moving the configuration from noxfile.py environment variables to tests/unit/conftest.py. The reviewer suggests using monkeypatch.setattr instead of manually setting and resetting _helpers.ENABLE_AFE_SERVER_TIMING to leverage pytest's built-in teardown mechanism and prevent state leakage.

Comment on lines +34 to 42
monkeypatch.setenv("SPANNER_DISABLE_AFE_SERVER_TIMING", "true")
_helpers.ENABLE_AFE_SERVER_TIMING = False
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)
yield
# Reset singletons after test to ensure no leakage
_helpers.ENABLE_AFE_SERVER_TIMING = False
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)

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.

medium

Instead of manually setting and resetting _helpers.ENABLE_AFE_SERVER_TIMING directly, use monkeypatch.setattr to leverage pytest's built-in teardown mechanism. This is more idiomatic and guarantees that the original state is restored automatically after each test, preventing potential state leakage.

Suggested change
monkeypatch.setenv("SPANNER_DISABLE_AFE_SERVER_TIMING", "true")
_helpers.ENABLE_AFE_SERVER_TIMING = False
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)
yield
# Reset singletons after test to ensure no leakage
_helpers.ENABLE_AFE_SERVER_TIMING = False
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)
monkeypatch.setenv("SPANNER_DISABLE_AFE_SERVER_TIMING", "true")
monkeypatch.setattr(_helpers, "ENABLE_AFE_SERVER_TIMING", False)
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)
yield
# Reset singletons after test to ensure no leakage
SpannerMetricsTracerFactory._metrics_tracer_factory = None
SpannerMetricsTracerFactory._current_metrics_tracer_ctx.set(None)

…n conftest

Addresses review comment on PR #17980 by using monkeypatch.setattr(_helpers, 'ENABLE_AFE_SERVER_TIMING', False) instead of direct mutation, allowing pytest to automatically restore state on teardown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant