Bug 180390

Summary:

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

Product:

WebKit

Reporter:

Aakash Jain <aakash_jain>

Component:

Tools / Tests

Assignee:

Aakash Jain <aakash_jain>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, ap, clopez, commit-queue, dbates, dean_johnson, lforschler, ossy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=180389
https://bugs.webkit.org/show_bug.cgi?id=195557
https://bugs.webkit.org/show_bug.cgi?id=217831

Attachments:

Description	Flags
Proposed patch	none
Updated patch	none
Updated patch	none

Aakash Jain

Reported 2017-12-04 18:03:01 PST

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'

Attachments
Proposed patch (3.52 KB, patch) 2017-12-04 18:11 PST, Aakash Jain	no flags	Details Formatted Diff Diff
Updated patch (4.51 KB, patch) 2018-02-26 16:25 PST, Aakash Jain	no flags	Details Formatted Diff Diff
Updated patch (4.60 KB, patch) 2018-03-16 14:38 PDT, Aakash Jain	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Aakash Jain

Comment 1 2017-12-04 18:11:35 PST

Created attachment 328420 [details] Proposed patch

Aakash Jain

Comment 2 2017-12-04 18:18:40 PST

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

Csaba Osztrogonác

Comment 3 2017-12-05 09:37:01 PST

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

Csaba Osztrogonác

Comment 4 2017-12-05 09:52:43 PST

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

Comment 5 2018-02-26 16:25:32 PST

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 6 2018-03-05 16:21:32 PST

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

Comment 7 2018-03-16 14:38:58 PDT

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 8 2018-04-17 17:04:19 PDT

Comment on attachment 335970 [details] Updated patch Looks good to me. Makes the unit test work back.

WebKit Commit Bot

Comment 9 2018-04-24 13:15:08 PDT

Comment on attachment 335970 [details] Updated patch Clearing flags on attachment: 335970 Committed r230969: <https://trac.webkit.org/changeset/230969>

WebKit Commit Bot

Comment 10 2018-04-24 13:15:10 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 11 2018-04-24 13:16:21 PDT

<rdar://problem/39695069>

Note You need to log in before you can comment on or make changes to this bug.