Extended environment variable control - Githubissues

avocado-framework / avocado

Avocado is a set of tools and libraries to help with automated testing. One can call it a test framework with benefits. Native tests are written in Python and they follow the unittest pattern, but any executable can serve as a test.

https://avocado-framework.github.io/

Other

336 stars 335 forks source link

Extended environment variable control #5901

Closed richtja closed 2 months ago

richtja commented 2 months ago

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 commented 2 months ago

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.

richtja commented 2 months ago

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

richtja commented 2 months ago

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

dgibson commented 2 months ago

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.

richtja commented 2 months ago

Hi @clebergnu and @dgibson I have updated this PR, to cover also option (1) from https://github.com/avocado-framework/avocado/pull/5901#discussion_r1569931373. 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

richtja commented 2 months ago

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

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

dgibson commented 2 months ago

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

richtja commented 2 months ago

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

dgibson commented 2 months ago

@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 commented 2 months ago

@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.