Skip to content

refactor(rules): extract duplicated stubbings in InputMatcherTest into 3 helpers - #1204

Merged
ginccc merged 1 commit into
labsai:mainfrom
mia-li-001:refactor-duplicated-stubbings-3
Nov 3, 2026
Merged

ginccc merged 1 commit into
labsai:mainfrom
mia-li-001:refactor-duplicated-stubbings-3

Conversation

@mia-li-001

@mia-li-001 mia-li-001 commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

I am a researcher at the university. In my analysis of the project, I observed that

In the test class InputMatcherTest, 3 groups of stubbings are repeated across tests:

  1. The tests execute_occurrence_anyStep and execute_occurrence_never define the same stubbings:
    • when(previousConversationStep1.getLatestData(eq(EXPRESSIONS_PARSED.key()))).thenAnswer(invocation -> new Data<>("expressions", "someNonMatchingExpression"))
    • when(previousConversationStep2.getLatestData(eq(EXPRESSIONS_PARSED.key()))).thenAnswer(invocation -> new Data<>("expressions", inputExpressions))
    • when(memory.getAllSteps()).thenAnswer(invocation -> new ConversationMemory.ConversationStepStack(Arrays.asList(previousConversationStep1, previousConversationStep2)))
  2. The tests execute_occurrence_currentStep and execute_occurrence_lastStep define the same stubbings:
    • when(conversationStep.getLatestData(eq(EXPRESSIONS_PARSED))).thenAnswer(invocation -> new Data<>("expressions", inputExpressions))
    • when(expressionProvider.parseExpressions(eq(inputExpressions))).thenAnswer(invocation -> expectedInputExpressions)
  3. The tests execute_occurrence_anyStep and execute_occurrence_never define the same stubbings:
    • when(expressionProvider.parseExpressions(eq(inputExpressions))).thenAnswer(invocation -> expectedInputExpressions)
    • when(expressionProvider.parseExpressions(not(eq(inputExpressions)))).thenAnswer(invocation -> new Expressions())

Here previousConversationStep1, previousConversationStep2, memory, conversationStep and expectedInputExpressions stand for the mocks and values that differ from test to test.

These duplicated stubbings repeat the setup in each test, so a change to that setup has to be made in 2 places for each group. I propose extracting them into 3 private helper methods:

  • setupIConversationMemoryGetAllStepsAndGetLatestDataStubs, which takes 3 parameters (previousConversationStep1, previousConversationStep2 and memory) and is called from 2 tests.
  • setupConversationStepGetLatestDataAndExpressionProviderParseExpressionsStubs, which takes 2 parameters (conversationStep and expectedInputExpressions) and is called from 2 tests.
  • setupExpressionProviderParseExpressionsStubs, which takes 1 parameter (expectedInputExpressions) and is called from 2 tests.

In execute_occurrence_currentStep and execute_occurrence_lastStep, the getLatestData stubbing now comes after the getCurrentStep/getPreviousSteps stubbing (inside the helper call). All stubbings are still made before the matcher runs, so the behavior is the same.

This refactoring also reduces the cognitive complexity of InputMatcherTest by 13.3% (from 309.1 to 268.1, measured with Genese). Cognitive complexity, introduced by SonarSource, estimates how hard code is for a human to understand.

The change only moves the existing stubbings into the helpers. The stubbed calls, arguments and returned values are the same as before, and the test results are unchanged.

I propose a solution below to remove the duplicated stubbings. Happy to modify the pull request based on your feedback.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • ♻️ Refactoring (no functional changes)
  • 🔧 Chore (dependency updates, CI changes, etc.)

Related Issue

N/A

Closes #

Changes Made

  • Extracted 3 groups of identical Mockito stubbings in InputMatcherTest.java into 3 new private helper methods:
  • setupIConversationMemoryGetAllStepsAndGetLatestDataStubs(final IConversationStep previousConversationStep1, final IConversationStep previousConversationStep2, final IConversationMemory memory), called from execute_occurrence_anyStep and execute_occurrence_never.
  • setupConversationStepGetLatestDataAndExpressionProviderParseExpressionsStubs(final IConversationStep conversationStep, final Expressions expectedInputExpressions), called from execute_occurrence_currentStep and execute_occurrence_lastStep.
  • setupExpressionProviderParseExpressionsStubs(final Expressions expectedInputExpressions), called from execute_occurrence_anyStep and execute_occurrence_never.
  • Replaced the duplicate setup code in the 4 test methods with calls to the extracted helper methods.
  • In execute_occurrence_currentStep and execute_occurrence_lastStep, the getLatestData stubbing now comes after the getCurrentStep/getPreviousSteps stubbing (inside the helper call); all stubbings are still made before the matcher runs.
  • Preserved exact stubbing behavior, invocation arguments, and test assertions without altering test semantics.

How to Test

  1. Run the target test class using Maven wrapper:

    ./mvnw test -Dtest=InputMatcherTest
    
  2. Verify that all 13 tests pass without regressions:

    Tests run: 13, Failures: 0, Errors: 0, Skipped: 0
    
  3. (Optional) Run the full test suite locally:

    ./mvnw clean verify -DskipITs
    

Checklist

  • My code follows the project's code style
  • I have added tests that prove my fix/feature works
  • Existing tests pass locally (./mvnw clean verify -DskipITs)
  • I have updated documentation if needed
  • My changelog entry is a new file under docs/changelog.d/ — not an edit to docs/changelog.md, which every open PR would conflict over
  • My commit messages follow conventional commits
  • I have not committed any secrets, API keys, or tokens
  • This PR has a clear, focused scope (one concern per PR)

Summary by CodeRabbit

  • Tests
    • Consolidated repeated test setup across input-matching scenarios while preserving coverage for matching and nonmatching expressions.

@vercel

vercel Bot commented Nov 1, 2026

Copy link
Copy Markdown

@mia-li-001 is attempting to deploy a commit to the eddi Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: labsai/EDDI
  • Branch: main
  • File: .coderabbit.yaml
  • Commit: 01c3250
  • Layers: Repository YAML
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e82dafcb-bc40-447f-b29d-eb2a582c7780
Commits

Reviewing files that changed from the base of the PR and between 01c3250 and cae1167.

Files selected for processing: 1
  • src/test/java/ai/labs/eddi/modules/rules/impl/conditions/InputMatcherTest.java

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

The InputMatcherTest class replaces repeated Mockito stubs with shared helpers for conversation-step data and parsed-expression provider responses.

Changes: InputMatcher test setup

Layer / File(s) Summary
Shared test setup
src/test/java/ai/labs/eddi/modules/rules/impl/conditions/InputMatcherTest.java
The current-step, last-step, any-step, and never-occurrence tests use shared helpers for stubbing step data and provider responses. Nonmatching expressions continue to receive an empty result.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Refactor

Merge Risk: Minimal · up to cae11

The helper extraction preserves the existing test behavior and changes no production code. No actionable merge-blocking risk remains, subject to normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: extracting duplicated Mockito stubbings in InputMatcherTest into three helpers.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@ginccc

ginccc commented Nov 1, 2026

Copy link
Copy Markdown
Member

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor
Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ginccc ginccc left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution, @mia-li-001!

@ginccc
ginccc merged commit 1e8ecd3 into labsai:main Nov 3, 2026
31 of 35 checks passed
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