Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub workflow update (test multiple Bitcoin Core versions) #1218

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

kristapsk
Copy link
Member

@kristapsk kristapsk commented Apr 3, 2022

  • Remove Python 3.6 (EOL), add Python 3.10.Done in CI: Remove Python 3.6 (EOL), add 3.10 #1409
  • Add support for testing against multiple Bitcoin Core versions, add 0.18.0 (oldest officially support for test suite) and 22.025.0 (latest stable release). Previously we tested against hardcoded 0.19.1. IMO testing against oldest supported and latest makes sense, if tests pass on both, they should pass on releases inbetween too, unless there is some behaviour affecting us changed twice.

Also changed Core downloads from bitcoin.org to bitcoincore.org.

For now, opening PR as a test and for discussion. Will increase running time 2x.

@kristapsk kristapsk force-pushed the github-workflow-update branch 2 times, most recently from e4a4d23 to 77ff139 Compare April 3, 2022 17:33
@kristapsk
Copy link
Member Author

Another thing we don't test is aarch64 (arm64). That would also double number of checks, but no point even trying that right now as ARM tests are currently broken.

@kristapsk
Copy link
Member Author

Will increase running time 2x.

Actually not, as they run in parallel. Question is - what limits do we have here? I have no idea.

@kristapsk kristapsk force-pushed the github-workflow-update branch from b976e5d to 9ac7610 Compare April 7, 2022 17:02
@kristapsk kristapsk changed the title GitHub workflow update (Python and Bitcoin Core versions) [test] GitHub workflow update (Python and Bitcoin Core versions) Apr 7, 2022
@kristapsk kristapsk force-pushed the github-workflow-update branch from 9ac7610 to 7a5b593 Compare April 7, 2022 17:15
@AdamISZ
Copy link
Member

AdamISZ commented Apr 8, 2022

Will increase running time 2x.

Actually not, as they run in parallel. Question is - what limits do we have here? I have no idea.

Yeah I'm really uncertain about what's going on here. Builds in parallel, sure, but still .. compute cost? Also, is caching somehow applied? I somehow got that impression a while back, because I saw some CI test runs finish really quickly, whereas on my machine (which is fairly high end), it was taking 14 minutes or so. I guess caching could work if: there are no edits to jmdaemon then maybe it doesn't run the tests in there? I guess that should make sense with well designed tests - the tests should test specifically and only the contents of the package, and any external dependencies should be somehow mocked. But I don't know the dividing line between theory and practice there.

Certainly if we started thinking about all the different configurations in which you could run this application, there would be an exponential blowup in the number of test runs.

We should probably using mocking more aggressively than we do in our tests ... or should we? - that would just stop us observing how our code fails to run with Bitcoin Core version XX - the RPC interface to Core is a moving target.

I guess that's just part of the whole integration/e2e testing vs unit testing thing. We need both.

@kristapsk
Copy link
Member Author

It looks there are 2000 free CI/CD minutes, so don't think there is a problem with this change (testing with oldest supported and most recent Bitcoin Core and supported Python versions).

image

@kristapsk kristapsk force-pushed the github-workflow-update branch 2 times, most recently from 996952a to 2e390ff Compare October 23, 2022 21:00
@kristapsk
Copy link
Member Author

Updated actions/cache from v2 to v3.

@kristapsk kristapsk force-pushed the github-workflow-update branch from 2e390ff to d0b1824 Compare October 27, 2022 13:31
@kristapsk
Copy link
Member Author

Updated Bitcoin Core from 22.0 to 23.0.

We could also add macos-latest in addition to ubuntu-latest, with some small changes. I recently did that for my bitcoin-scripts project. See kristapsk/bitcoin-scripts@2eedf0a.

@kristapsk kristapsk force-pushed the github-workflow-update branch from d0b1824 to dcbec3c Compare November 5, 2022 13:27
@kristapsk
Copy link
Member Author

Rebased on current master, to include update to pytest 6.2.5.

kristapsk added a commit that referenced this pull request Dec 15, 2022
b59fdcd Remove Python 3.6 (EOL), add 3.10 (Kristaps Kaupe)

Pull request description:

  Subset of #1218. Python 3.6 is not only EOL for a long time, also no more available on ubuntu-latest for GitHub workflows, see https://github.com/JoinMarket-Org/joinmarket-clientserver/actions/runs/3705429480/jobs/6279325863. So this is kinda urgent, our CI is broken otherwise.

Top commit has no ACKs.

Tree-SHA512: 9438202d142666736548872a6850d8c72edcfca09d30ed34ecc7290dfe26e1f3af15860cdafa60cd321321666ee9d44a7a3be54931c808f076e1fe89cf2ccb31
@kristapsk kristapsk force-pushed the github-workflow-update branch from dcbec3c to c84019f Compare December 16, 2022 12:38
@kristapsk kristapsk changed the title GitHub workflow update (Python and Bitcoin Core versions) GitHub workflow update (test multiple Bitcoin Core versions) Dec 16, 2022
@kristapsk
Copy link
Member Author

Rebased and changed newest tested Core version to 24.0.1.

@kristapsk
Copy link
Member Author

Rebased

@kristapsk kristapsk force-pushed the github-workflow-update branch from 0185828 to 68844a8 Compare June 3, 2023 11:39
* Add support for testing against multiple Bitcoin Core versions, add
  0.18.0 (oldest officially support for test suite) and 25.0 (latest
  stable release). Previously we tested against hardcoded 0.19.1. IMO
  testing against oldest supported and latest makes sense, if tests pass
  on both, they should pass on releases inbetween too, unless there is
  some behaviour affecting us changed twice.
* Update actions/cache from v2 to v3.

Also changed Core downloads from bitcoin.org to bitcoincore.org.
@kristapsk kristapsk force-pushed the github-workflow-update branch from 68844a8 to c921206 Compare June 3, 2023 11:42
@kristapsk
Copy link
Member Author

Rebased and updated bitcoind from 24.0.1 to 25.0.

@AdamISZ
Copy link
Member

AdamISZ commented Aug 7, 2023

@kristapsk I guess you are waiting for an ACK on this, only? I would leave this to your discretion, if you think it is a net positive for the CI, then please go ahead. I see nothing wrong in the script,so:

ACK c921206

@kristapsk
Copy link
Member Author

@kristapsk I guess you are waiting for an ACK on this, only? I would leave this to your discretion, if you think it is a net positive for the CI, then please go ahead. I see nothing wrong in the script,so:

ACK c921206

Yes, was waiting for a Concept ACK, don't see downsides here.

@kristapsk kristapsk merged commit 5eccf3b into JoinMarket-Org:master Aug 7, 2023
@kristapsk kristapsk deleted the github-workflow-update branch August 7, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants