Bug 90284 – [GTK] WebKit test runner ignores all system environment variables

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GTK] WebKit test runner ignores all system environment variables

[Xabier Rodríguez Calvar]

Created attachment 150189 [details]
Patch

[Xabier Rodríguez Calvar]

I don't know if the environment variables are ignored on purpose for some reason, but I cooked this patch because this way we still overwrite the ones that we want and let through the system ones that we do not want to touch.

Somebody told me that Philippe is one of the python guys, so I am also adding him to CC.

[Philippe Normand]

I think it's ok to have total, explicit, control over the environment variables as it's currently done.

Maybe an alternative could be that you pass vars to run-webkit-tests with a specific prefix (like WEBKIT_TESTS_ or something similar) and handle thses internally, instead of passing the whole os.environ, which can have, I think, unexpected side-effects.

[Martin Robinson]

Comment on attachment 150189 [details]
Patch

This seems reasonable enough to me. Philippe?

[Martin Robinson]

(In reply to comment #3)
> I think it's ok to have total, explicit, control over the environment variables as it's currently done.
> 
> Maybe an alternative could be that you pass vars to run-webkit-tests with a specific prefix (like WEBKIT_TESTS_ or something similar) and handle thses internally, instead of passing the whole os.environ, which can have, I think, unexpected side-effects.

Hrm. Now that Philippe mentions it, it is quite useful to have complete control over the environment. What environment variables are you wanting to pass through? It might make sense to special case them.

[Xabier Rodríguez Calvar]

GST_PLUGIN_SYSTEM_PATH so that I can avoid GStreamer to load a certain plugin that it is breaking my tests.

Sometimes it could be useful to let some variables thru, so I think the idea of preprending WEBKIT_TESTS_ is good as you only pass the variables you have the intention to pass.

[Dirk Pranke]

Comment on attachment 150189 [details]
Patch

Philippe is right. This is the opposite of what you're supposed to be doing; we don't want unexpected variables to have an effect. Feel free to add the ones you want to pass through, but we should be able to see a deterministic list of what can affect the runner.

[Philippe Normand]

Dirk,

Would that alternative to allow prefixed variables be accepted in the WebKitPort class? Maybe it would be useful for other ports but I understand if such workaround can be limited to the GTK port only :)

[Dirk Pranke]

Well, we're moving to combine WebKitPort and Port, so all ports will be WebKitPorts. I'm not wild about the prefix idea, because it still suffers from the same problem (you can't look at the code and figure out which variables will have an effect).

I would be open to the idea of a command line flag that lets you pass arbitrary environment variables through; I can certainly see the value for testing and development in not having to add a variable to the whitelist every time you try something, but if code gets checked in using a new variable, it should probably have to be in the list, I think.

I could be convinced I'm wrong, though.

[Xabier Rodríguez Calvar]

Created attachment 151087 [details]
Patch

Now only variables prefixed with WEBKIT_TESTS_ are passed to the tests (by removing the prefix first). I hope this makes the process enough sandboxed but with some flexibility. If you like the proposed solution I can write some tests and submit a final patch for review.

[Dirk Pranke]

(In reply to comment #10)
> Created an attachment (id=151087) [details]
> Patch
> 
> Now only variables prefixed with WEBKIT_TESTS_ are passed to the tests (by removing the prefix first). I hope this makes the process enough sandboxed but with some flexibility. If you like the proposed solution I can write some tests and submit a final patch for review.

As I said above, I'm not wild about WEBKIT_TESTS_, either. Do you really need to be able to update arbitrary environment variables? Is there a reason you don't just add the variables you care about to the whitelist, either in base.py or override setup_environ_for_server in gtk.py if only gtk cares about GST_PLUGIN_SYSTEM_PATH (which seems likely)?

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> As I said above, I'm not wild about WEBKIT_TESTS_, either. Do you really need to be able to update arbitrary environment variables? Is there a reason you don't just add the variables you care about to the whitelist, either in base.py or override setup_environ_for_server in gtk.py if only gtk cares about GST_PLUGIN_SYSTEM_PATH (which seems likely)?

Because the problem happens only in my setup and I need to workaround it, but it is not a general problem. If it were I would have whitelisted the variable.

Phil thought that there must be a better solution that would be adding an extra parameter when running the test runner, for example --extra-env="...=..." and I also think that that's a better solution, so I will go for this and resubmit the patch when ready, if you like the approach.

[Dirk Pranke]

(In reply to comment #12)
> Phil thought that there must be a better solution that would be adding an extra parameter when running the test runner, for example --extra-env="...=..." and I also think that that's a better solution, so I will go for this and resubmit the patch when ready, if you like the approach.

Yes, a command line flag would be fine. We've generally used --additional-XXX for this sort of flag, so something like --additional-env-var foo=bar would be great.

[Xabier Rodríguez Calvar]

Created attachment 152729 [details]
Patch

This patch adds the --addional-env-var arguments for the test runner. The tests work in my case the problem is solved if I use the right env var. I fixed the webkit py unit tests that were broken and added a new one for the new feature.

[Dirk Pranke]

Comment on attachment 152729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152729&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:787
> +        for string_variable in self._options.additional_env_var:

You should probably use self.get_option('additional_env_var', []) here; that should eliminate most (if not all) of the concerns about having to ensure additonal_env_var is defined in the options object.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:62
> +        return Port(host, config=port_config, options=port_options)

Why don't you need to propagate the other **kwargs ?

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_unittest.py:37
> +from webkitpy.tool.mocktool import MockOptions

nit: I'm trying to move away from using MockOptions. import optparse and use optparse.Values() instead, since it's a built-in class in the standard library that does the same thing.

> Tools/Scripts/webkitpy/tool/mocktool.py:51
> +        self.ensure_value('additional_env_var', [])

don't do this, it's a bad way of letting additional hidden dependencies creep in. tests that need this value to be set should be explicit about it (yes, that's kind of a hassle).

[Xabier Rodríguez Calvar]

Created attachment 153264 [details]
Patch

It looks like Dirk's comments worked like a charm and all the problems with the dependencies of the base unit tests are gone. Normal and base unit tests work and the patch looks much better.

[Dirk Pranke]

Comment on attachment 153264 [details]
Patch

cleaned up nicely. thanks!

[WebKit Review Bot]

Comment on attachment 153264 [details]
Patch

Clearing flags on attachment: 153264

Committed r123132: <http://trac.webkit.org/changeset/123132>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.