Bug 180389 - [build.webkit.org] Simplify imports in mastercfg_unittest.py
Summary: [build.webkit.org] Simplify imports in mastercfg_unittest.py
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-04 17:50 PST by Aakash Jain
Modified: 2019-03-11 09:24 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (5.72 KB, patch)
2017-12-04 17:57 PST, Aakash Jain
clopez: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2017-12-04 17:50:03 PST
mastercfg_unittest.py  has a BuildBotConfigLoader class which is used to import master.cfg. We should use python imports directly. All the code in this class is no longer required. e.g.: _add_webkitpy_to_sys_path method is unnecessary as Buildbot doesn't depend on webkitpy at all. We should remove this class.
Comment 1 Aakash Jain 2017-12-04 17:57:34 PST
Created attachment 328419 [details]
Proposed patch
Comment 2 Csaba Osztrogonác 2017-12-05 09:39:35 PST
Comment on attachment 328419 [details]
Proposed patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:-32
> -    def _add_dependant_modules_to_sys_modules(self):
> -        from webkitpy.thirdparty.autoinstalled import buildbot
> -        sys.modules['buildbot'] = buildbot

This code was to be able to run mastercfg_unittest.py without installing buildbot and its dependencies.
webkitpy autoinstaller installs exactly the same buildbot and twisted version as the build.webkit.org uses.
Comment 3 Aakash Jain 2017-12-05 16:05:08 PST
Good point. I think I need to look into webkitpy more and understand why does webkitpy has buildbot code. Maybe we can move that autoinstaller code here.
Comment 4 Carlos Alberto Lopez Perez 2018-04-17 17:05:46 PDT
Comment on attachment 328419 [details]
Proposed patch

I'm setting r- because this patch not longer applies on trunk. Also it looks a similar thing has been done on bug 180390. Should we just close this?