| Message ID | 20180518144500.31436-2-quentin.schulz@bootlin.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,v2,1/2] cmd: nvedit: add whitelist option for env import | expand |
On 05/18/2018 08:45 AM, Quentin Schulz wrote: > This tests that the importing of an environment with a specified > whitelist works as intended. > > If the variable whitelisted_vars is not set, the env importing should > fail with a given message. > > For each variable separated by spaces in whitelisted_vars, if > - foo is bar in current env and bar2 in exported env, after importing > exported env, foo shall be bar2, > - foo does not exist in current env and foo is bar2 in exported env, > after importing exported env, foo shall be bar2, > - foo is bar in current env and does not exist in exported env (but is > in the whitelisted_vars), after importing exported env, foo shall be > empty, > > Any variable not in whitelisted_vars should be left untouched. Acked-by: Stephen Warren <swarren@nvidia.com> > diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py > +def test_env_import_whitelist(state_test_env): > + state_test_env.u_boot_console.run_command('env import -w %s' % addr) > + > + validate_set(state_test_env, "foo1", "bar1") > + validate_set(state_test_env, "foo2", "bar2") > + validate_set(state_test_env, "foo3", "bar3") > + validate_empty(state_test_env, "foo4") It might make sense to iterate over *all* variables and make sure that they have the same value before/after the import. But, the current code seems to cover the basic cases, so it's probably not strictly necessary to do that.
Hi Stephen, On Fri, May 18, 2018 at 10:04:05AM -0600, Stephen Warren wrote: > On 05/18/2018 08:45 AM, Quentin Schulz wrote: > > This tests that the importing of an environment with a specified > > whitelist works as intended. > > > > If the variable whitelisted_vars is not set, the env importing should > > fail with a given message. > > > > For each variable separated by spaces in whitelisted_vars, if > > - foo is bar in current env and bar2 in exported env, after importing > > exported env, foo shall be bar2, > > - foo does not exist in current env and foo is bar2 in exported env, > > after importing exported env, foo shall be bar2, > > - foo is bar in current env and does not exist in exported env (but is > > in the whitelisted_vars), after importing exported env, foo shall be > > empty, > > > > Any variable not in whitelisted_vars should be left untouched. > > Acked-by: Stephen Warren <swarren@nvidia.com> > > > diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py > > > +def test_env_import_whitelist(state_test_env): > > > + state_test_env.u_boot_console.run_command('env import -w %s' % addr) > > + > > + validate_set(state_test_env, "foo1", "bar1") > > + validate_set(state_test_env, "foo2", "bar2") > > + validate_set(state_test_env, "foo3", "bar3") > > + validate_empty(state_test_env, "foo4") > > It might make sense to iterate over *all* variables and make sure that they > have the same value before/after the import. But, the current code seems to > cover the basic cases, so it's probably not strictly necessary to do that. I would advocate for as many test sets as possible, each one testing a very specific scenario. That way, it's easy to know which scenario failed instead of having to find within a big scenario what failed and why. Here, I just want to test that importing some env variables work. It would make sense to have a small test that tests that setting an environment variable works (that what's done with test_env_set* test functions I think) and others to test the exporting of variables and the importing of some others with all the possible/impossible arguments of these cli commands. I wish I could test this importing without manually exporting an environnement before to reduce "dependencies" of the test and thus keep the test scenario to the actual command we're testing. However the more the arguments of the commands, the more functions to write ("exponentially") as we have to test each and every case. The best would be to also have tests for expected and wanted failures. I think it's a good start for now but more needs to be done in the env testing regarding (at least) env exporting and importing. Quentin
On 18 May 2018 at 08:45, Quentin Schulz <quentin.schulz@bootlin.com> wrote: > This tests that the importing of an environment with a specified > whitelist works as intended. > > If the variable whitelisted_vars is not set, the env importing should > fail with a given message. > > For each variable separated by spaces in whitelisted_vars, if > - foo is bar in current env and bar2 in exported env, after importing > exported env, foo shall be bar2, > - foo does not exist in current env and foo is bar2 in exported env, > after importing exported env, foo shall be bar2, > - foo is bar in current env and does not exist in exported env (but is > in the whitelisted_vars), after importing exported env, foo shall be > empty, > > Any variable not in whitelisted_vars should be left untouched. > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- > > added in v2 > > test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org>
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index b7f960c755..d0a2e36876 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -6,6 +6,7 @@ # Test operation of shell commands relating to environment variables. import pytest +import u_boot_utils # FIXME: This might be useful for other tests; # perhaps refactor it into ConsoleBase or some other state object? @@ -231,3 +232,42 @@ def test_env_expansion_spaces(state_test_env): unset_var(state_test_env, var_space) if var_test: unset_var(state_test_env, var_test) + +def test_env_import_whitelist_empty(state_test_env): + """Test trying to import 0 variables from an environment""" + ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console) + addr = '%08x' % ram_base + + set_var(state_test_env, "foo1", "bar1") + + state_test_env.u_boot_console.run_command('env export %s' % addr) + + c = state_test_env.u_boot_console + with c.disable_check('error_notification'): + response = c.run_command('env import -w %s' % addr) + assert(response.startswith('## Error: whitelisted_vars is not set')) + +def test_env_import_whitelist(state_test_env): + """Test importing only a handful of env variables from an environment""" + ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console) + addr = '%08x' % ram_base + + #no foo1 in current env, foo2 overridden, foo3 should be of the value + #before exporting and foo4 should be deleted + set_var(state_test_env, "whitelisted_vars", "foo1 foo2 foo4") + set_var(state_test_env, "foo1", "bar1") + set_var(state_test_env, "foo2", "bar2") + set_var(state_test_env, "foo3", "bar3") + + state_test_env.u_boot_console.run_command('env export %s' % addr) + + unset_var(state_test_env, "foo1") + set_var(state_test_env, "foo2", "test2") + set_var(state_test_env, "foo4", "bar4") + + state_test_env.u_boot_console.run_command('env import -w %s' % addr) + + validate_set(state_test_env, "foo1", "bar1") + validate_set(state_test_env, "foo2", "bar2") + validate_set(state_test_env, "foo3", "bar3") + validate_empty(state_test_env, "foo4")
This tests that the importing of an environment with a specified whitelist works as intended. If the variable whitelisted_vars is not set, the env importing should fail with a given message. For each variable separated by spaces in whitelisted_vars, if - foo is bar in current env and bar2 in exported env, after importing exported env, foo shall be bar2, - foo does not exist in current env and foo is bar2 in exported env, after importing exported env, foo shall be bar2, - foo is bar in current env and does not exist in exported env (but is in the whitelisted_vars), after importing exported env, foo shall be empty, Any variable not in whitelisted_vars should be left untouched. Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> --- added in v2 test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)