Skip to content

Conversation

@AndreaBozzo
Copy link

  • Add test_subscribe_batch: tests LogScanner::subscribe_batch() with HashMap
  • Add test_project_by_name: tests TableScan::project_by_name() with column names
  • Both tests include error case validation

Linked issue: #31

Add two new integration tests to improve API coverage:
- test_subscribe_batch: tests LogScanner::subscribe_batch() with HashMap
- test_project_by_name: tests TableScan::project_by_name() with column names

Both tests include error case validation.
@AndreaBozzo
Copy link
Author

AndreaBozzo commented Dec 27, 2025

Good morning and thanks for running the workflows!

i updated the assertion to value in list_offsets with commit 9cc0250 + a clippy fix on table.rs with df7c410, all tests now pass locally

Note: clippy is pointing many more warnings in other tests, while this is still fine, its something that should be examinated in the future

Comment on lines 461 to 465
assert!((1..=6).contains(&id), "id should be between 1 and 6");
assert!(
["a", "b", "c", "d", "e", "f"].contains(&value),
"value should be one of a-f"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the ordering of the records?

Copy link
Author

Choose a reason for hiding this comment

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

Agree

good catch, thank you

sort records by offset and verify exact ordering.

let records: Vec<_> = scan_records.into_iter().collect();
let mut records: Vec<_> = scan_records.into_iter().collect();
records.sort_by_key(|r| r.offset());
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, is this sort_by_key needed? What happens if you do not sort?

My assumption is that poll_batch should already return records ordered by offset.

@AndreaBozzo
Copy link
Author

The sort_by_key is needed because poll() does not guarantee ordering as records may arrive from different buckets or be processed asynchronously. Also thank you again on this becouse that's how i noticed the pattern!

This follows the existing pattern in the codebase:

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.

2 participants