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

Implemented test for unauthenticated input notes #408

Merged
merged 14 commits into from
Jul 19, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Jul 12, 2024

We implemented unauthenticated input notes support in these PRs: #390 and #396.

We need to add tests to make sure the unauthenticated note handling in the whole pipeline (from posting transaction to block building) works as expected.

I've also refactored some methods and improved some mocks needed for tests.

One of tests fails at the moment, will try to find a bug.

Resolves #398

@polydez polydez requested a review from bobbinth July 12, 2024 14:04
@polydez polydez marked this pull request as ready for review July 17, 2024 06:50
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

I think this looks good though I'm not 100% familiar with all the corner cases yet.

I left a few test naming nits/suggestions - feel free to ignore since its a style thing, but will elaborate below.

In general the tests name the condition being tested, but not the expected result. So as someone unfamiliar with the code I need to read the test carefully to know what's happening.

As an example:

fn missing_notes()

versus

fn block_with_missing_notes_is_rejected()

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a coupe of comments inline - but they are all pretty minor.

One general thing (not specific to this PR): I find tests in this repo pretty hard to read and follow. We should probably start thinking about refactoring of the testing framework soon. Let's create an issue for this.

Comment on lines +247 to +248
#[cfg(test)]
mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's add TESTS section separator above this line.

fn test_output_note_tracker_duplicate_output_notes() {
let mut txs = mock_proven_txs();

OutputNoteTracker::new(&txs).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line do here? Are we just making sure that the set of transactions returned from mock_proven_txs() is valid? If so, we should document this, or maybe even have a separate test for this.

Comment on lines 366 to 367
assert_eq!(batch.input_notes.len(), expected_input_notes.len());
assert_eq!(batch.input_notes, expected_input_notes);
Copy link
Contributor

Choose a reason for hiding this comment

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

The check for the lengths to match seems a bit redundant as if the actual vectors don't match, the lengths won't match either.

@polydez polydez merged commit aab0b58 into next Jul 19, 2024
6 checks passed
@polydez polydez deleted the polydez-unauth-note-tests branch July 19, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants