Page MenuHomePhabricator

Make all automated commits run smoke tests prior to pushing
Needs ReviewPublic

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

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
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
Branch
smoke-tests-2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9554
Build 17015: Bitcoin ABC Buildbot
Build 17014: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Wed, Feb 12, 00:38
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Feb 12, 00:38
Fabien requested changes to this revision.Thu, Feb 13, 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.Thu, Feb 13, 10:03
jasonbcox added inline comments.Thu, Feb 13, 20:29
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.

jasonbcox updated this revision to Diff 16456.Fri, Feb 21, 18:05

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.Sat, Feb 22, 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.Sat, Feb 22, 12:41
jasonbcox added inline comments.Mon, Feb 24, 22:34
contrib/source-control-tools/automated-commits.sh
125 ↗(On Diff #16456)

Yes

jasonbcox updated this revision to Diff 16483.Mon, Feb 24, 22:42

Fix unit tests