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.

Sign up for GitHub

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

Jump to bottom

Add test for inserting anchor without tx #961

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add test for inserting anchor without tx #961

wants to merge 4 commits into from
+88 −3

Conversation

remix7531
Copy link
Contributor

Description

This adds a test case described in issue958

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

evanlinjin
evanlinjin requested changes May 3, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thanks for the work so far. Remember that we also want to test recovering the appended additions into a new graph. Refer to #958 (comment)

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
@remix7531 remix7531 requested a review from evanlinjin May 8, 2023 12:58
@remix7531 remix7531 force-pushed the issue958 branch 2 times, most recently from e4bdd6d to df0b541 Compare May 9, 2023 13:12
evanlinjin
evanlinjin requested changes May 9, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good work so far. Few notes.

  1. Ensure CI passes.
  2. Ensure GitHub can verify your commits (Maybe you need to give your PGP or SSH public key to GitHub?).
  3. Please rebase your commits into one.
  4. Use loops where you can (for example if you are inserting multiple anchors or multiple txs).

@remix7531 remix7531 requested a review from evanlinjin May 15, 2023 11:16
danielabrozzoni
danielabrozzoni reviewed May 16, 2023
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

A few improvements can be made:

  • Fix the warnings cargo clippy is throwing
  • Use a more descriptive commit message. Instead of "tests for issue958", describe which test you're adding in the title, and add "Partially fixes More tests for IndexedTxGraph and TxGraph #958" in the description. In general, it should be possible to understand what a commit is doing from the title/description without having to open github

The rest looks good to me.

@remix7531
Adds test inserting anchor without tx
0536bd5 
Test inserting anchors without the corresponding transaction
and testing the recoverability of the transaction graph

Partially fixes "More tests for IndexedTxGraph and TxGraph bitcoindevkit#958"
bitcoindevkit#958
evanlinjin
evanlinjin requested changes May 17, 2023
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thank you again, sorry I think it will be better to reduce the scope of this test.

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory added this to the 1.0.0-alpha.1 milestone May 23, 2023
@remix7531 remix7531 requested a review from evanlinjin May 23, 2023 15:46
@remix7531 remix7531 requested a review from evanlinjin June 5, 2023 12:57
@notmandatory
Copy link
Member

Since it looks like still a bit of work today I'm moving this out of the alpha.1 milestone.

@notmandatory notmandatory removed this from the 1.0.0-alpha.1 milestone Jun 19, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.2 milestone Jun 20, 2023
@remix7531
Copy link
Contributor Author

I do not understand what is missing here. The workflows failed because of this
error: package log v0.4.19 cannot be built because it requires rustc 1.60.0 or newer, while the currently active rustc version is 1.57.0

Just tested the workflows locally and it seams to work now.

@notmandatory notmandatory mentioned this pull request Jul 15, 2023
Closed
19 tasks
@danielabrozzoni
Copy link
Member

Hey @remix7531, sorry for the late response! I think you encountered a problem with our CI, that's why it would work locally but fail here. Can you try rebasing?

@oleonardolima
Copy link
Contributor

Is this something still needed for 1.0.0-beta milestone ? (cc @notmandatory @ValuedMammal)

I got a bit confused because I noticed that the #958 issue already has been closed but without the tests covered by this PR.

@ValuedMammal
Copy link
Contributor

I think it wouldn't hurt to have a test similar to this

@ValuedMammal ValuedMammal mentioned this pull request Nov 20, 2024
Open
@notmandatory
Copy link
Member

This testing could help with verifying sqlite schema question in #1712

@notmandatory notmandatory mentioned this pull request Nov 21, 2024
Draft
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlinjin evanlinjin Awaiting requested review from evanlinjin

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

Requested changes must be addressed to merge this pull request.

Assignees

@remix7531 remix7531

Labels
module-blockchain tests
Projects
BDK
Status: In Progress
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.
7 participants
@remix7531 @notmandatory @danielabrozzoni @oleonardolima @ValuedMammal @evanlinjin @nondiremanuel