Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extended environment variable control #5901

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Extended environment variable control #5901

merged 4 commits into from
Apr 29, 2024

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Apr 11, 2024

This PR adds more control over the environment variables in exec-tests for users. It brings a way how to disable one env variable or clear the whole environment during the test runtime so it won't be available to test script.

@richtja richtja added enhancement customer:Passt Requirements/issues raised by the Passt project labels Apr 11, 2024
@richtja richtja added this to the #105 - Codename TBD milestone Apr 11, 2024
@richtja richtja requested a review from clebergnu April 11, 2024 13:29
@richtja richtja self-assigned this Apr 11, 2024
@richtja richtja linked an issue Apr 11, 2024 that may be closed by this pull request
Extended environment variable control #5889
Closed
@richtja
Copy link
Contributor Author

richtja commented Apr 11, 2024

Hi @dgibson, please have a look and let me know if this satisfies pasta needs from #5901. You can look at documentation which describes environment variable control.

clebergnu
clebergnu requested changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja,

Thanks for working on this. I've left my feedback in individual comments.

Thanks!

docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/writing.rst Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
avocado/plugins/runners/exec_test.py Outdated Show resolved Hide resolved
avocado/plugins/runners/exec_test.py Outdated Show resolved Hide resolved
@richtja richtja force-pushed the env_manipulation branch 2 times, most recently from b1625d7 to 5a6f95d Compare April 16, 2024 14:09
@richtja richtja requested a review from clebergnu April 16, 2024 14:31
@richtja
Copy link
Contributor Author

richtja commented Apr 16, 2024

Hi @clebergnu I have applied changes based on your proposals. Please have a look.

clebergnu
clebergnu requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja,

Thanks for this second version! I've spotted a few extra things though.

docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
docs/source/guides/writer/chapters/basics.rst Outdated Show resolved Hide resolved
avocado/core/__init__.py Outdated Show resolved Hide resolved
@richtja
Copy link
Contributor Author

richtja commented Apr 17, 2024

Hi @clebergnu thank you very much for your review, It was really helpful. I addressed your comments in force-push. Please have a look.

@richtja richtja requested a review from clebergnu April 17, 2024 09:20
@dgibson
Copy link
Contributor

dgibson commented Apr 18, 2024

Hi @dgibson, please have a look and let me know if this satisfies pasta needs from #5901. You can look at documentation which describes environment variable control.

So far, we don't really have much in the way of requirements here. I suggested something like this to Cleber, because I think long term it's something we'll want, and makes for a more generally useful way of running these "extended exec" tests from Avocado. I'll review as best I can.

dgibson
dgibson reviewed Apr 18, 2024
View reviewed changes
avocado/core/nrunner/runnable.py Show resolved Hide resolved
avocado/plugins/runners/exec_test.py Show resolved Hide resolved
selftests/functional/basic.py Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
avocado/core/__init__.py Show resolved Hide resolved
avocado/plugins/runners/exec_test.py Show resolved Hide resolved
clebergnu
clebergnu approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

This version LGTM. Before merging, I'm asking @dgibson to ACK it too, since he participated a lot in the review and has great interest in the actual result.

@clebergnu clebergnu requested a review from dgibson April 25, 2024 02:16
@richtja richtja requested a review from clebergnu April 25, 2024 12:48
@richtja
Copy link
Contributor Author

richtja commented Apr 25, 2024

Hi @clebergnu and @dgibson I have updated this PR, to cover also option (1) from #5901 (comment). Now the runner.exectest.clear_env takes two values all and system, for clearing the whole environment or only the non-avocado variables. Please have a look, thanks

dgibson
dgibson reviewed Apr 26, 2024
View reviewed changes
Copy link
Contributor

@dgibson dgibson left a comment

Choose a reason for hiding this comment

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

In terms of the design and functionality exposed this LGTM.

I still see a few nits in the implementation, for which I've left comments.

avocado/plugins/runners/exec_test.py Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
avocado/plugins/runners/exec_test.py Show resolved Hide resolved
selftests/functional/basic.py Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
selftests/functional/basic.py Outdated Show resolved Hide resolved
@richtja
[Doc update] move exec-test doc to its own chapter
d4e2cde 
Signed-off-by: Jan Richter <[email protected]>
@richtja
[Doc update] Exec-test environment variables
01d9d1c 
This commit updates avocado documentation to describe defining
environment variables in exec-tests by user.

Signed-off-by: Jan Richter <[email protected]>
@richtja
exec-test disable env variable
e27f4c5 
This commit adds more control over the environment variables in
exec-tests for users. It brings a way how to disable env variable during
the test runtime so it won't be available to test script. Only thing
which user needs to do is to set the variable to `None` in test
`kwargs`.

Reference: avocado-framework#5889
Signed-off-by: Jan Richter <[email protected]>
@richtja
Copy link
Contributor Author

richtja commented Apr 26, 2024

Hi @dgibson, thank you for your review. I did an update base on your comments. Please have a look.

  1. Validation for runner.exectest.clear_env values.
  2. Updated test message to Script unexpectedly passed with environment variable set.
  3. Spurious space removal moved to correct commit.

clebergnu
clebergnu approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@richtja richtja requested a review from dgibson April 26, 2024 13:19
@dgibson
Copy link
Contributor

dgibson commented Apr 26, 2024

Updates LGTM. Only remaining nit is the question about whether we're really testing the clear_env = all case.

@richtja
exec-test clear testing environment
112dd07 
This commit adds more control over the environment variables in
exec-tests for users. It brings a way how to clear the test environment
during the test runtime so it won't affect the test script. Only thing
which user needs to do is to set the runner.exectest.clear_env to
`system` or `all` in tests config.

Reference: avocado-framework#5889
Signed-off-by: Jan Richter <[email protected]>
@richtja
Copy link
Contributor Author

richtja commented Apr 26, 2024

Hi @dgibson I have split the EXEC_ENV_VARIABLE_TEST script into two to make the tests more transparent. Plase have a look.

@dgibson
Copy link
Contributor

dgibson commented Apr 29, 2024

@richtja well.. I'm trying to ack the latest version, but github appears to be messing up. I hit "submit review" and the cursor changes, but it never appears to actually go through.

In any case, it LGTM now. Thanks for the EXEC_ENV_VARIABLE_TEST - I did miss the fact that it was already testing that, but I think it's clearer with this change in any case.

@richtja
Copy link
Contributor Author

richtja commented Apr 29, 2024

@richtja well.. I'm trying to ack the latest version, but github appears to be messing up. I hit "submit review" and the cursor changes, but it never appears to actually go through.

In any case, it LGTM now. Thanks for the EXEC_ENV_VARIABLE_TEST - I did miss the fact that it was already testing that, but I think it's clearer with this change in any case.

Thank you, I am merging it now.

@richtja richtja merged commit b337ef3 into avocado-framework:master Apr 29, 2024
60 checks passed
@richtja richtja deleted the env_manipulation branch October 9, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clebergnu clebergnu clebergnu approved these changes

@dgibson dgibson Awaiting requested review from dgibson

Assignees

@richtja richtja

Labels
customer:Passt Requirements/issues raised by the Passt project enhancement
Projects
Default project
Archived in project
Milestone
#105 - Poor Things
Development

Successfully merging this pull request may close these issues.

Extended environment variable control
3 participants
@richtja @dgibson @clebergnu