Add test to verify if all homu builders are listed in the buildbot config #571

charlesvdv · 2017-01-02T19:49:14Z

That should resolve the #443.

This change is

highfive · 2017-01-02T19:49:17Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

charlesvdv · 2017-01-02T20:37:00Z

The tests are failing because buildbot is installed in the python3 library folder and not the python2 but buildbot is a python2 package which is required to import master.cfg. I don't really know what solutions could be the best:

symlink
install buildbot with pip2 (is Salt even support this ? With a quick look at the doc I don't see any ways to change the pip version)
add a sys.path.append before the dynamic import

Or maybe another idea I didn't think of ?

aneeshusa

The approach overall looks good; thanks for tackling a less-easy issue!

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/get_buildbot_cfg.py

+import sys
+
+if __name__ == '__main__':
+    sys.path.append('/home/servo/buildbot/master/')


Pull this path out into a constant.

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/get_buildbot_cfg.py

+
+    config = imp.load_source(
+        'config',
+        '/home/servo/buildbot/master/master.cfg'


Use os.path.join to join the constant and master.cfg.

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/get_buildbot_cfg.py

+    for sched in config.c['schedulers']:
+        if sched.name == 'servo-auto':
+            out = {'builders': sched.builderNames}
+            print(json.dumps(out))


Add a return 0 after printing to indicate success and exit early. After the loop (meaning servo-auto was not found, print an error message to sys.stderr (e.g. error: 'servo-auto' scheduler not found) and add a return 1`.

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/valid_builders.py

+from tests.util import Failure, Success
+
+
+def is_builders_valid(buildbot, homu):


grammar: is_builders_valid -> are_builders_valid

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/valid_builders.py

+    homu_buildbot = homu_cfg['repo']['servo']['buildbot']
+
+    ret = subprocess.run(
+        ['python2', 'tests/sls/homu/get_buildbot_cfg.py'],


You'll need to use /usr/bin/python2. This is because Travis's only has one "mega" Trusty image, on which it installs all its Pythons, Rubies, etc., and the $PATH always contains all of those Pythons before the main /usr/bin, etc. entries. Hence, just python2 invokes a Python from /opt, not the main Python that we want.

(This will fix the tests currently failing.)

Also, right now the rest of the test suite is agnostic to CWD, so I'd prefer to refer to the get_buildbot_cfg.py file differently, likely using __file__.

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/valid_builders.py

+        )
+    if not is_builders_valid(buildbot_builders, homu_buildbot['try_builders']):
+        return Failure(
+            'Homu config is not in sync with the buildbot config', ''


Have this and the previous message be different (ie. reference builders vs try_builders) to the error message is immediately helpful.

Additionally, we should make use of the second argument to Failure. To check validity, cast the homu builders and buildbot builders to lists, then subtract the sets to perform a diff. If the len() of the diff is non-zero, then pretty-print the mismatched (i.e. remaining in the diff) values to use for the second argument to Failure().

aneeshusa · 2017-01-02T21:31:51Z

tests/sls/homu/valid_builders.py

+            'Homu config is not in sync with the buildbot config', ''
+        )
+
+    return Success('Buildbot and homu configs are synced.')


nit: No period at the end

aneeshusa

Good progress! I've left some comments to make this code more Pythonic.
Also, please rebase on top of the latest master to pull in #573.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/get_buildbot_cfg.py

+BUILDBOT_MASTER_PATH = '/home/servo/buildbot/master/'
+
+if __name__ == '__main__':
+    sys.path.append(BUILDBOT_MASTER_PATH)


Let's move this code into a main() function and use return instead of exit in main(); here it can be called as sys.exit(main()).

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/get_buildbot_cfg.py

+            print(json.dumps(out))
+            exit(0)
+
+    sys.stderr.write('error: "servo-auto" scheduler not found\n')


Prefer print(..., file=sys.stderr).

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+
+
+def get_script_path():
+    dirname = os.path.dirname(__file__)


This can be inlined into the next line (should fit within 80 char limit).

Also, the whole method can then be inlined into run().

It would also be nice to add a comment about why we need to invoke a Python2 subprocess to run the separate script.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+
+def are_builders_valid(buildbot, homu_cfg, buildertype):
+    for builder in homu_cfg[buildertype]:
+        if builder not in buildbot:


The set differencing subsumes the need to loop over each builder and check if it is present or not, so these first two lines aren't needed.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+from tests.util import Failure, Success
+
+
+def are_builders_valid(buildbot, homu_cfg, buildertype):


Inline are_builders_valid into run(), which allows removing a bunch of duplicated error checking.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+def are_builders_valid(buildbot, homu_cfg, buildertype):
+    for builder in homu_cfg[buildertype]:
+        if builder not in buildbot:
+            diff = list(set(homu_cfg[buildertype]) - set(buildbot))


No need to cast to a list.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+            diff = list(set(homu_cfg[buildertype]) - set(buildbot))
+            diff_print = ''
+            if len(diff) != 0:
+                diff_print = 'Difference: {}'.format(diff)


This can be inlined into the Failure constructor.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+                diff_print = 'Difference: {}'.format(diff)
+
+            fail = Failure(
+                'Homu "{}" config isn\'t sync with buildbot config'


nit: sync -> synced
Also, using double quotes for this string will make it easier to read (no need to escape the apostrophe).

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+
+def run():
+    homu_cfg = toml.load('/home/servo/homu/cfg.toml')
+    homu_buildbot = homu_cfg['repo']['servo']['buildbot']


I think homu_builders is more clear.

aneeshusa · 2017-01-06T05:29:16Z

tests/sls/homu/valid_builders.py

+
+    buildbot_builders = json.loads(ret.stdout.decode('utf-8'))['builders']
+
+    resp = are_builders_valid(buildbot_builders, homu_buildbot, 'builders')


You can use a loop to cut down on some duplication:

for builder_type in ['builders', 'try_builders']: diff = .... if diff: return Failure(...) return Success(..)

charlesvdv · 2017-01-06T11:48:22Z

@aneeshusa This should be OK now!

Sorry for the long review process 😞

aneeshusa

I was testing this out and we can make the error reporting more helpful. Currently, we early return if any builder set has invalid builders, but we should buffer all differences and return them together at the end.

aneeshusa · 2017-01-07T08:48:08Z

tests/sls/homu/valid_builders.py

+
+    buildbot_builders = json.loads(ret.stdout.decode('utf-8'))['builders']
+
+    for buildertype in ['builders', 'try_builders']:


nit: buildertype -> builder_set

aneeshusa · 2017-01-07T08:48:08Z

tests/sls/homu/valid_builders.py

+        diff = set(homu_builders[buildertype]) - set(buildbot_builders)
+        if diff:
+            return Failure(
+                "Homu {} config isn't synced with buildbot config"


nits:

add a colon at the end

capitalize Buildbot

aneeshusa · 2017-01-07T08:48:08Z

tests/sls/homu/valid_builders.py

+            return Failure(
+                "Homu {} config isn't synced with buildbot config"
+                .format(buildertype),
+                'Difference: {}'.format(diff)


Before the for loop, initialize a string var (e.g. failure_msg) to an empty string. In the loop, if the if diff: condition is true, append information about the diff to failure_msg.

After the loop, if failure_msg then return a Failure passing failure_msg for the second argument. This way if there invalid builders for builders and try_builders they're both reported.

Prose changes:

Keep the same title (first arg) for the Failure, but don't mention the builder set

Use a more descriptive prefix than Difference, maybe Invalid builders for {builder_set}: (this gets added to failure_msg iteratively)

aneeshusa · 2017-01-07T08:48:08Z

tests/sls/homu/valid_builders.py

+    )
+    if ret.returncode != 0:
+        return Failure(
+            'Unable to retrieve buildbot builders name:', ret.stderr


nit: Use 'Unable to retrieve Buildbot builders:'

aneeshusa · 2017-01-07T08:48:08Z

tests/sls/homu/valid_builders.py

+    homu_builders = homu_cfg['repo']['servo']['buildbot']
+
+    # We need to invoke a new process to read the buildbot master config
+    # because buildbot is written in python2.


nit: capitalize both instances of Buildbot

aneeshusa

Sorry about the long review, you're almost there!

aneeshusa · 2017-01-10T00:32:10Z

tests/sls/homu/valid_builders.py

+        if diff:
+            if failure_msg:
+                failure_msg += '\n'
+            failure_msg += 'Invalid builders for "{}": {}'.format(builder_set, diff)


Linter says this line is too long.

aneeshusa · 2017-01-10T00:32:10Z

tests/sls/homu/valid_builders.py

+        diff = set(homu_builders[builder_set]) - set(buildbot_builders)
+        if diff:
+            if failure_msg:
+                failure_msg += '\n'


You can actually include the \n unconditionally as part of the format string - the outputter is smart and will strip the last trailing newline.


        Add test to verify if all homu builders are valid

aneeshusa · 2017-01-11T06:46:23Z

Thanks for tackling this! @bors-servo r+

bors-servo · 2017-01-11T06:46:24Z

📌 Commit 16d83db has been approved by aneeshusa

bors-servo · 2017-01-11T06:46:27Z

⚡ Test exempted - status


        Auto merge of #571 - charlesvdv:validbuilders, r=aneeshusa

Add test to verify if all homu builders are listed in the buildbot config That should resolve the #443.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/571)

highfive assigned aneeshusa Jan 2, 2017

highfive added the S-awaiting-review label Jan 2, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch 2 times, most recently from 9eb326e to 4c42c60 Jan 2, 2017

aneeshusa suggested changes Jan 2, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 2, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 3, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch from de9f6a4 to 2d1da66 Jan 3, 2017

aneeshusa suggested changes Jan 6, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 6, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch from 2d1da66 to 16b13b5 Jan 6, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 6, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch from 16b13b5 to 00ecc87 Jan 6, 2017

aneeshusa suggested changes Jan 7, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 7, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch from 00ecc87 to 5ab75c2 Jan 7, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 7, 2017

aneeshusa suggested changes Jan 10, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 10, 2017

charlesvdv force-pushed the charlesvdv:validbuilders branch from 5ab75c2 to 16d83db Jan 10, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 10, 2017

Add test to verify if all homu builders are valid

Loading status checks…

16d83db

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 11, 2017

bors-servo merged commit 16d83db into servo:master Jan 11, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test exempted
Details

highfive removed the S-awaiting-merge label Jan 11, 2017

aneeshusa mentioned this pull request Jan 11, 2017

Ensure all Buildbot builders listed for servo repo in Homu configuration are valid #443

Closed

		from tests.util import Failure, Success


		def is_builders_valid(buildbot, homu):

		from tests.util import Failure, Success


		def are_builders_valid(buildbot, homu_cfg, buildertype):


		buildbot_builders = json.loads(ret.stdout.decode('utf-8'))['builders']

		resp = are_builders_valid(buildbot_builders, homu_buildbot, 'builders')


		buildbot_builders = json.loads(ret.stdout.decode('utf-8'))['builders']

		for buildertype in ['builders', 'try_builders']:

servo / saltfs

Join GitHub today

Add test to verify if all homu builders are listed in the buildbot config #571

Add test to verify if all homu builders are listed in the buildbot config #571

Conversation

charlesvdv commented Jan 2, 2017 • edited by larsbergstrom

highfive commented Jan 2, 2017

charlesvdv commented Jan 2, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa Jan 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

charlesvdv commented Jan 6, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

charlesvdv commented Jan 2, 2017 •

edited by larsbergstrom

aneeshusa Jan 2, 2017 •

edited