Page MenuHomePhabricator

Make all automated commits run smoke tests prior to pushing
ClosedPublic

Authored by jasonbcox on Feb 12 2020, 00:38.

Details

Summary

It's a good idea to make sure that the happy path build is green and some unit tests and functional tests pass.
It's not feasible to test everything, as we don't want automatic commits to take a long time, otherwise we risk
merge conflicts. But, we can be reasonably confident that nothing is catastrophically broken, and CI will pick up the
rest once the change is detected by Teamcity.

Depends on D5269

Test Plan

See D5269

Diff Detail

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

Event Timeline

Fabien requested changes to this revision.Feb 13 2020, 10:03
Fabien added a subscriber: Fabien.
Fabien added inline comments.
contrib/source-control-tools/automated-commits.sh
109 ↗(On Diff #16310)

Smoke tests is not very meaningful to someone editing the build-configurations.sh script.
build-automated-commits-tests or similar is more relevant.

contrib/teamcity/build-configurations.sh
203 ↗(On Diff #16310)

Thinking more about this it might not even be at the right place.

What is the point of having an new build type in the CI, that reports status (what does the status mean ?) for automated commits ?

This revision now requires changes to proceed.Feb 13 2020, 10:03
contrib/source-control-tools/automated-commits.sh
109 ↗(On Diff #16310)

This is true, but limiting the scope of those tests doesn't seem necessary when that build config can be used in other places in the future. See my other comment.

contrib/teamcity/build-configurations.sh
203 ↗(On Diff #16310)

The intent was this could be reused in places other than just automated commits, but it does not need to run in CI on every commit. The other existing builds that complete rather quickly (just a couple minutes) do not have good general test coverage because they're focusing on something specific (build-without-wallet, build-werror, etc).

IMO the name build-smoke-tests is self explanatory when thought about in a general context.

Revised design for smoke tests:

  • Decouple from build-configurations.sh since smoke tests do not have a direct relationship to CI.
  • Smoke tests do not need to live in their own script since they are not universally applicable anyway.
  • Leverage build_cmake which was recently moved out of contrib/teamcity (since it's not CI-specific).
Fabien requested changes to this revision.Feb 22 2020, 12:41
Fabien added inline comments.
contrib/source-control-tools/automated-commits.sh
125 ↗(On Diff #16456)

I suppose what you want here is ninja check-bitcoin ?

This revision now requires changes to proceed.Feb 22 2020, 12:41
contrib/source-control-tools/automated-commits.sh
125 ↗(On Diff #16456)

Yes

Fabien added inline comments.
contrib/source-control-tools/automated-commits.sh
126 ↗(On Diff #16483)

You can speed this up a bit with ninja check-bitcoin check-functional so they can be parallelized.

This revision is now accepted and ready to land.Feb 25 2020, 09:03