Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Conversation

@xJonathanLEI
Copy link
Contributor

Red CI leaves newcomers a really bad impression :(

What this PR does:

  1. Fixes a wrong auth separator of . instead of :.
  2. Adds a Bitcoin Core regtest node in CI because some tests rely on a functional Bitcoin RPC node to run.
  3. Excludes the get_zkapps test on CI as it hard-codes a testnet address. (IMO ideally this test would become end-to-end so this would no longer be an issue)

Even with changes proposed here, CI is still red, but that's because of clippy. The testing step is actually successful. Clippy issues should be addresses in a separate PR.

@xJonathanLEI xJonathanLEI changed the title fix: wrong auth separator ci: run bitcoin core regest node for tests Jan 24, 2024
@xJonathanLEI xJonathanLEI changed the title ci: run bitcoin core regest node for tests ci: run bitcoin core regtest node for tests Jan 24, 2024
@xJonathanLEI
Copy link
Contributor Author

Btw I tried fixing the get_zkapps test to be end-to-end and generates its own utxo, but then the issue becomes a race condition between get_zkapps and test_generate_transaction_flow... as they both try to use the same utxo for sending the tx.

Need to restructure the tests a bit more to fully resolve.

args: run --all-features --release
# Test `get_zkapps` is skipped on CI for now as it uses a hard-coded testnet address
# TODO: remove this exclusion once the test becomes end-to-end
args: run --all-features --release -E "package(zkbitcoin) - test(get_zkapps)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add #[ignore] under the #[test] of the test also

Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is AMAZING! thank-you so much :) maybe we should add some doc in DEVELOPER.md on how to run the tests locally with regtest as well!

@mimoo mimoo merged commit 56a36a0 into CharmsDev:main Jan 27, 2024
@xJonathanLEI xJonathanLEI deleted the ci/fix_failed_tests branch January 27, 2024 06:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants