Skip to content

chore: clean environment for errorreporting tests#16736

Merged
ohmayr merged 1 commit intomainfrom
fix-errorreporting-tests
Apr 21, 2026
Merged

chore: clean environment for errorreporting tests#16736
ohmayr merged 1 commit intomainfrom
fix-errorreporting-tests

Conversation

@ohmayr
Copy link
Copy Markdown
Contributor

@ohmayr ohmayr commented Apr 20, 2026

clean environment for errorreporting tests

@ohmayr ohmayr requested a review from a team as a code owner April 20, 2026 23:07
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

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 updates the unit tests for the Error Reporting client by clearing environment variables during the test_ctor_defaults test case to ensure isolation from the host system. The review feedback suggests applying the environment clearing decorator at the class level rather than to a single method to improve maintainability and ensure consistency across all tests in the suite.

self.assertEqual(len(positional), 1)
return positional[0]

@mock.patch.dict(os.environ, clear=True)
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

The pull request title mentions cleaning the environment for 'tests' (plural), but this decorator is only applied to test_ctor_defaults. If other tests in this class also require a clean environment to ensure isolation from the host system, it would be more maintainable and consistent to apply the @mock.patch.dict(os.environ, clear=True) decorator at the class level instead of repeating it for individual test methods. This aligns with the principle of applying configuration changes consistently across relevant tests.

References
  1. Changes to configurations should be applied consistently across all relevant parts of the codebase.

Copy link
Copy Markdown
Contributor

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM

@ohmayr ohmayr merged commit 1678f41 into main Apr 21, 2026
30 checks passed
@ohmayr ohmayr deleted the fix-errorreporting-tests branch April 21, 2026 10:24
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.

3 participants