Skip to content

Conversation

@neekolas
Copy link
Contributor

@neekolas neekolas commented Nov 18, 2025

Add a Tokio async test validating reply-to-attachment behavior in xmtp_mls::groups::message_list using Group::find_messages_v2

Add a unit test in message_list.rs that stores an attachment, stores a reply referencing its hex id, queries via Group::find_messages_v2, and asserts the reply links to the attachment and exposes expected text and attachment fields.

📍Where to Start

Start with the tests::test_find_messages_reply_to_attachment test in message_list.rs.


Macroscope summarized 93a6620.

Copy link
Contributor Author


How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @neekolas's task —— View job


Code Review - November 18, 2025, 23:17 UTC

Tasks:

  • Read the changed file to understand the test implementation
  • Analyze the test code against best practices and project conventions
  • Provide review feedback

Review Feedback

The new test test_find_messages_reply_to_attachment (lines 627-690) looks solid overall. Here are a few observations:

Positive aspects:

  • Good test coverage: Tests the important case of replying to attachments with proper validation of both the reply and referenced attachment content
  • Clear assertions: Uses existing helper functions (assert_message_count, assert_reply_references) consistently with other tests in the file
  • Proper setup: Follows the established pattern with setup_test_group() and create_and_store_message()

Minor issues:

1. Inconsistent test attribute (line 627)

#[tokio::test]

According to CLAUDE.md, tests should use #[xmtp_common::test] instead of #[tokio::test] to ensure they run in both native and WebAssembly environments.

2. Redundant content access (lines 682-686)

if let MessageBody::Text(text) = reply.content.as_ref() {
    assert_eq!(text.content, "Reply text content");
}

This assertion accesses reply.content a second time after already matching on it at line 668. The reply's actual text content appears to be embedded in the Reply variant itself, not in a separate Text variant. This logic seems incorrect and would likely panic with "Expected text content in reply".

Suggested fix for issue #2:
The reply content should be validated through the Reply structure's own fields rather than trying to extract it as Text. Check how TestContentGenerator::reply_content() constructs the reply to understand what fields are available on the Reply variant.


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