[buildbot] Introduce selecting diff builds based on filename regex
ClosedPublic
Actions

Authored by jasonbcox on Nov 19 2020, 18:03.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC96022ff9077a: [buildbot] Introduce selecting diff builds based on filename regex

Summary

As this codebase turns more into a mono-repo, we need a way to reduce
CI load since the number of patches is expected to grow by an order of magnitude.

A regex that matches changed filenames is a quick and easy way to determine which tests
may be valuable to run. While this isn't perfect, we can get a huge payoff almost
immediately. Consider this recent example where D8441 ran a bunch of node-related tests
when it clearly is not related in any way. If we expect 3+ devs to work on website(s)
then we cannot tie up the CI running node tests when there is no behavior change.

In addition, this allows us to be more aggressive in running wider-coverage of tests
for specific types of changes. This patch demonstrates that by adding buildbot as a
diff-enabled build. Before this patch, running the buildbot tests would have been a
waste of CI resources as it would run on every revision. After this patch, that is no
longer the case.

Updates to other build configurations will come in future patches so they can be reviewed
more carefully on their own.

Depends on D8473

Test Plan

pytest

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Nov 19 2020, 18:03

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 19 2020, 18:03

jasonbcox requested review of this revision.Nov 19 2020, 18:03

jasonbcox edited the summary of this revision. (Show Details)Nov 19 2020, 18:08

jasonbcox added a child revision: D8472: [buildbot] Support a list of regex when matching builds to changed files.Nov 19 2020, 18:19

Harbormaster completed remote builds in B14116: Diff 25888.Nov 19 2020, 18:48

I suggest you wrap the differential.getcommitpaths call to a phabricator_wrapper method. This allows for unit testing it, makes it easier to mock and easier to factorize in the future if needed.
Also general style nit, python tends to prefer snake_case.

contrib/buildbot/server.py
265 ↗	(On Diff #25888)	TIL
271 ↗	(On Diff #25888)	If you set the `runOnDiffRegex`, then you don't need to `runOnDiff` which is redundant
276 ↗	(On Diff #25888)	`changedFiles.values()` makes more sense here since you don't need the key
contrib/teamcity/build-configurations.yml
407 ↗	(On Diff #25888)	Nit: you don't need the `.*`

This revision now requires changes to proceed.Nov 19 2020, 18:54

jasonbcox mentioned this in D8473: [buildbot] Add a facility for fetching the files changed in a given revision.Nov 19 2020, 21:04

Changes according to feedback, and rebased on D8473

jasonbcox edited the summary of this revision. (Show Details)Nov 19 2020, 22:38

jasonbcox added a parent revision: D8473: [buildbot] Add a facility for fetching the files changed in a given revision.

Harbormaster completed remote builds in B14120: Diff 25896.Nov 19 2020, 22:45

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.

jasonbcox mentioned this in rABCcd684dad5ea7: [buildbot] Add a facility for fetching the files changed in a given revision.Nov 20 2020, 00:20

Fabien accepted this revision.Nov 20 2020, 14:35

Fabien added inline comments.

contrib/buildbot/server.py
281 ↗	(On Diff #25896)	Nit: maybe this can be simplified a bit by making the regex always valid ? diffRegex = v.get('runOnDiffRegex', None) or (r".*" if v.get('runOnDiff', False) else r"^$") Feel free to ignore if you think it makes it less readable.