Skip to content

illmock: test reason for message: notification#637

Open
adamdickmeiss wants to merge 2 commits into
mainfrom
illmock-reason-notification-test
Open

illmock: test reason for message: notification#637
adamdickmeiss wants to merge 2 commits into
mainfrom
illmock-reason-notification-test

Conversation

@adamdickmeiss

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings June 17, 2026 08:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Not ready to approve

The newly added notification test currently uses a status that would not trigger side effects even without the notification early-return, so it may not actually catch regressions.

Pull request overview

This PR updates the illmock requester-side ISO18626 handling to simplify state cleanup for Unfilled messages and adds a regression test intended to ensure ReasonForMessage=notification does not trigger side effects. It also modernizes many test assertions by replacing assert.Nil(err) with assert.NoError(err).

Changes:

  • Simplify TypeStatusUnfilled handling by always deleting the requester state (notification is already handled by an earlier return).
  • Update many tests to use assert.NoError for clearer error expectations.
  • Add a new test covering ReasonForMessage=notification confirmation behavior.
File summaries
File Description
illmock/app/requester.go Simplifies Unfilled branch by removing a redundant notification check and always deleting state for non-notification messages.
illmock/app/app_test.go Replaces many assert.Nil(err) with assert.NoError(err) and adds a new notification-focused regression test.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

Comment thread illmock/app/app_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants