Bug 101455 – webkitpy/layouttests integration test fails if high shards/processes environment variables are used

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dominik Röttsches (drott)]

I propose to skip those tests if the environment variables are set.

[Dominik Röttsches (drott)]

[522/1598] webkitpy.layout_tests.run_webkit_tests_integrationtest.MainTest.test_max_locked_shards failed:
  Traceback (most recent call last):
    File "/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py", line 315, in test_max_locked_shards
      self.assertTrue(any(['(1 locked)' in line for line in regular_output.buflist]))
  AssertionError: False is not true

is failing, the test itself looks like this:

    def test_max_locked_shards(self):
        if not self.should_test_processes:
            return
        _, _, regular_output, _ = logging_run(['--debug-rwt-logging', '--child-processes', '2'], shared_port=False)
        self.assertTrue(any(['(1 locked)' in line for line in regular_output.buflist]))

Shouldn't we use the --max-locked-shards switch here instead of making assumptions based on --child-processes?

[Dominik Röttsches (drott)]

Dirk, I am actually not sure what we should do with this test. Whenever I set something like
export WEBKIT_TEST_MAX_LOCKED_SHARDS=4

This test will fail. Changing the test to use the --max-locked-shards=1 parameter doesn't work either when the env var is set.

Can you suggest what we should do with this? I'd like to use the setting on our bots to improve layout test time, but currently it makes the python tests fail then.

[Dirk Pranke]

The intent of that test is to ensure that we still use only one locked shard by default even if we have more than one child process. We should probably document that with a comment or something :).

So, using --max-locked-shards would defeat the point of the test. I would suggest we mock out/override os.environ in this particular test to preserve the code path.

However, this also points out that we don't have tests ensuring that WEBKIT_TEST_MAX_LOCKED_SHARDS actually does do something, or ensuring that --max-locked-shards actually does the right thing. We should probably add tests for that as well.

I don't think we need to (or should) skip the tests.

Does that help?

[Dominik Röttsches (drott)]

Created attachment 173005 [details]
Patch, clearing env of WTMLS var.

[Dominik Röttsches (drott)]

(In reply to comment #3)
> The intent of that test is to ensure that we still use only one locked shard by default even if we have more than one child process. We should probably document that with a comment or something :).

I put a comment into the test.
> 
> So, using --max-locked-shards would defeat the point of the test. I would suggest we mock out/override os.environ in this particular test to preserve the code path.

Attaching a patch for this, hope it looks okay. I have a feeling this could be simpler.

> However, this also points out that we don't have tests ensuring that WEBKIT_TEST_MAX_LOCKED_SHARDS actually does do something, or ensuring that --max-locked-shards actually does the right thing. We should probably add tests for that as well.

Unfortunately I don't have time to create additional tests, but if you could r+ and land this fix, that'd be helpful for us to start using these env vars on our bots.

[Dirk Pranke]

Comment on attachment 173005 [details]
Patch, clearing env of WTMLS var.

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:323
> +            os.environ["WEBKIT_TEST_MAX_LOCKED_SHARDS"] = save_env_webkit_test_max_locked_shards

This should be in a try/catch block, otherwise if the test fails the variable won't be restored. Feel free to land it with that change.

[Dominik Röttsches (drott)]

Created attachment 173280 [details]
Always restore env.

[Dominik Röttsches (drott)]

(In reply to comment #7)
> Created an attachment (id=173280) [details]
> Always restore env.

Hopefully I understood you right and that's what you meant.

[WebKit Review Bot]

Comment on attachment 173280 [details]
Always restore env.

Clearing flags on attachment: 173280

Committed r134060: <http://trac.webkit.org/changeset/134060>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Comment on attachment 173280 [details]
Always restore env.

Yup, that's good. I meant to say try/finally and not try/catch :)