Skip to content

test: strengthen P1 effect regression assertions (#146, #162)#193

Open
sonupreetam wants to merge 1 commit into
unbound-force:mainfrom
sonupreetam:fix/146-strengthen-mutation-assertions
Open

test: strengthen P1 effect regression assertions (#146, #162)#193
sonupreetam wants to merge 1 commit into
unbound-force:mainfrom
sonupreetam:fix/146-strengthen-mutation-assertions

Conversation

@sonupreetam

@sonupreetam sonupreetam commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • go test -race -count=1 -short ./internal/analysis/ passes
  • All new assertions pass against existing implementation
  • CI green

Refs #146, #162

yvonnedevlinrh
yvonnedevlinrh previously approved these changes Jul 17, 2026

@yvonnedevlinrh yvonnedevlinrh 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.

LGTM - the count + target pattern is a real improvement over boolean presence checks.

Two observations:

  1. TestAnalyzeP1Effects_WrongWriteReturn — vacuously true assertions: HandleWrongWriteReturn produces zero effects, so both hasEffect checks pass trivially (an empty slice has no effects of any type). Consider adding if len(effects) != 0 { t.Errorf("expected no effects for WrongWriteReturn, got %d", len(effects)) } to explicitly assert the empty result. Without it, a future change that accidentally introduces a different effect type (e.g., some new call-expression effect) would pass undetected.

  2. Partial #162 coverage: This PR covers the partial method set case (PartialResponseWriter) and one near-miss Write signature (Write([]byte) int), but #162 also lists three other near-miss cases (Write(string), Write([]byte) (int64, error), Write([]byte, int)) and suggests an internal unit test for matchesWriteSignature directly. If #162 gets closed after this PR merges, those cases would be silently dropped. Might be worth either adding them here (they're ~5-line fixture types each) or noting in #162 which items remain open.

@sonupreetam

Copy link
Copy Markdown
Contributor Author

@yvonnedevlinrh Thank you for catching this. I did the following:

@sonupreetam
sonupreetam force-pushed the fix/146-strengthen-mutation-assertions branch from 21e2569 to 6c663a3 Compare July 20, 2026 11:29
…nbound-force#162)

- MapMutation/SliceMutation tests now assert exact count (1) and correct
  target name ("m"/"s") instead of boolean presence checks.
- Add PartialResponseWriter fixture (Write+Header, no WriteHeader) to
  verify WriterOutput is emitted but HTTPResponseWrite is not.
- Add WrongWriteReturn fixture (Write returns int, not (int, error)) to
  verify neither WriterOutput nor HTTPResponseWrite is emitted.

Refs unbound-force#146
Refs unbound-force#162

Signed-off-by: sonupreetam <spreetam@redhat.com>
@sonupreetam
sonupreetam force-pushed the fix/146-strengthen-mutation-assertions branch from 6c663a3 to 5e16f38 Compare July 20, 2026 11:54

@yvonnedevlinrh yvonnedevlinrh 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.

LGTM

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