Bug 180390 – [build.webkit.org] unit-tests fail when passwords.json is missing

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

[Aakash Jain]

unit-tests in mastercfg_unittest.py fail when passwords.json is missing. This file is not checked-in to the repository. So the unit-tests should be able to run without this file.

Exception:
[~/code/WebKit/Tools/BuildSlaveSupport/build.webkit.org-config]$python mastercfg_unittest.py 
Traceback (most recent call last):
  File "mastercfg_unittest.py", line 529, in <module>
    loadBuilderConfig(c)
  File "/Users/aakashjain/code/WebKit/Tools/BuildSlaveSupport/build.webkit.org-config/loadConfig.py", line 47, in loadBuilderConfig
    passwords = json.load(open('passwords.json'))
IOError: [Errno 2] No such file or directory: 'passwords.json'

[Aakash Jain]

Created attachment 328420 [details]
Proposed patch

[Aakash Jain]

This patch would apply to ToT after applying patch from https://bugs.webkit.org/show_bug.cgi?id=180389

[Csaba Osztrogonác]

Just a note, this test worked properly until your change https://trac.webkit.org/changeset/225433/webkit broke it.

[Csaba Osztrogonác]

Comment on attachment 328420 [details]
Proposed patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:531
> +    os.environ["BUILDBOT_TESTING"] = "true"
>      loadBuilderConfig(c)
> +    del os.environ["BUILDBOT_TESTING"]

Why an environment variable? 
I prefer a second is_test_mode_enabled parameter with default false.

[Aakash Jain]

Created attachment 334659 [details]
Updated patch

This patch also implements portion of https://bugs.webkit.org/attachment.cgi?id=328419&action=review

[Alexey Proskuryakov]

Comment on attachment 334659 [details]
Updated patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/loadConfig.py:48
> +    if is_test_mode_enabled:

I like it when names help achieve correct grammar. If the variable were called test_mode_is_enabled, then we'd have "if test_mode_is_enabled" here, which reads better.

> Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:-26
> -    def _mock_open(self, filename):

What was the problem with this? It seems like a nicer way to mock, as we don't need to pass additional arguments.

> Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:533
> +    BuildBotConfigLoader()._add_dependant_modules_to_sys_modules()

Not new with this patch, but s/dependant/dependent/.

[Aakash Jain]

Created attachment 335970 [details]
Updated patch

> I like it when names help achieve correct grammar. If the variable were called test_mode_is_enabled, then we'd have "if test_mode_is_enabled" here, which reads better.
Done

> Not new with this patch, but s/dependant/dependent/.
Replaced

> What was the problem with this? It seems like a nicer way to mock, as we don't need to pass additional arguments.
The globals() method to mock was working earlier when all the code was in master.cfg and we were doing execfile(master.cfg). It doesn't work anymore (after https://trac.webkit.org/r225433), since the file loading is now done in loadConfig.py, which doesn't have the same globals.

[Carlos Alberto Lopez Perez]

Comment on attachment 335970 [details]
Updated patch

Looks good to me. Makes the unit test work back.

[WebKit Commit Bot]

Comment on attachment 335970 [details]
Updated patch

Clearing flags on attachment: 335970

Committed r230969: <https://trac.webkit.org/changeset/230969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39695069>