Skip to content

[sonic-py-common] Clear environment variables before running device_info tests#7273

Merged
jleveque merged 2 commits intosonic-net:masterfrom
jleveque:py_common_test_sanitize_env
Jun 28, 2021
Merged

[sonic-py-common] Clear environment variables before running device_info tests#7273
jleveque merged 2 commits intosonic-net:masterfrom
jleveque:py_common_test_sanitize_env

Conversation

@jleveque
Copy link
Copy Markdown
Contributor

@jleveque jleveque commented Apr 9, 2021

Why I did it

To ensure any environment variables which are configured in the build/test environment do not influence the behavior of sonic-py-common during unit tests. For example, variables which might be set by continuous integration pipelines.

How I did it

Add class-scoped pytest fixture to TestDeviceInfo class which stashes the current environment variables, clears them and yields. Once all the test cases in the class finish, the fixture will restore the original environment variables.

Also remove unnecessary unittest-style setup and teardown functions from interface_test.py

How to verify it

Ensure unit tests pass during package build with the following commands:

PLATFORM=not_a_valid_platform_string make target/python-wheels/sonic_py_common-1.0-py2-none-any.whl
PLATFORM=not_a_valid_platform_string make target/python-wheels/sonic_py_common-1.0-py3-none-any.whl

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it seems doing something like tox. https://tox.readthedocs.io/en/latest/index.html#basic-example

tox will take care of environment isolation for you: it will strip away all operating system environment variables not specified via passenv. Furthermore, it will also alter the PATH variable so that your commands resolve first and foremost within the current active tox environment. In general all executables in the path are available in commands, but tox will emit a warning if it was not explicitly allowed via allowlist_externals.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. Do we want to introduce tox to our build process?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

are you only modify your own environ, why do you need to restore it? but again, i feel we are creating some virtual env here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, there is really no need to restore the old environment, it's just a best practice to leave things as you found them.

Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Apr 10, 2021

Choose a reason for hiding this comment

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

Back up

Mocking is a better way to create an isolated environment.
ref: https://adamj.eu/tech/2020/10/13/how-to-mock-environment-variables-with-pytest/ #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use mock in latest commit.

qiluo-msft
qiluo-msft previously approved these changes Apr 12, 2021
lguohan
lguohan previously approved these changes Jun 25, 2021
@jleveque jleveque dismissed stale reviews from lguohan and qiluo-msft via 835d591 June 26, 2021 02:04
@jleveque jleveque merged commit 9f114ca into sonic-net:master Jun 28, 2021
@jleveque jleveque deleted the py_common_test_sanitize_env branch June 28, 2021 16:30
qiluo-msft pushed a commit that referenced this pull request Jun 29, 2021
…nfo tests (#7273)

#### Why I did it

To ensure any environment variables which are configured in the build/test environment do not influence the behavior of sonic-py-common during unit tests. For example, variables which might be set by continuous integration pipelines.

#### How I did it

Add class-scoped pytest fixture to `TestDeviceInfo` class which stashes the current environment variables, clears them and yields. Once all the test cases in the class finish, the fixture will restore the original environment variables.

Also remove unnecessary unittest-style setup and teardown functions from interface_test.py
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…nfo tests (sonic-net#7273)

#### Why I did it

To ensure any environment variables which are configured in the build/test environment do not influence the behavior of sonic-py-common during unit tests. For example, variables which might be set by continuous integration pipelines.

#### How I did it

Add class-scoped pytest fixture to `TestDeviceInfo` class which stashes the current environment variables, clears them and yields. Once all the test cases in the class finish, the fixture will restore the original environment variables.

Also remove unnecessary unittest-style setup and teardown functions from interface_test.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants