Page MenuHomePhabricator

[buildbot] Introduce selecting diff builds based on filename regex

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



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

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien requested changes to this revision.Thu, Nov 19, 18:54
Fabien added a subscriber: Fabien.

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.

265 ↗(On Diff #25888)


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

407 ↗(On Diff #25888)

Nit: you don't need the .*

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

Changes according to feedback, and rebased on D8473

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
Fabien added inline comments.
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.

This revision is now accepted and ready to land.Fri, Nov 20, 14:35