Remaining TMS tasks#2685
Open
imashkanov wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOpenAPI ChangesOpenAPI Annotation Cleanup
Migration configuration
TMS test-case execution behavior and tests
Sequence Diagram(s)sequenceDiagram
participant TmsService as TmsTestCaseExecutionServiceImpl
participant StepExecRepo as TmsStepExecutionRepository
participant StepService as TmsStepExecutionService
participant ItemRepo as TestItemRepository
participant FinishHandler as FinishTestItemHandler
participant UpdateHandler as UpdateTestItemHandler
TmsService->>StepExecRepo: fetch/delete step executions (delete flow)
TmsService->>TmsService: delete TmsTestCaseExecution
TmsService->>ItemRepo: delete TEST item (cascade)
TmsService->>TmsService: evaluate requested StatusEnum
alt requested IN_PROGRESS or TO_RUN and current active
TmsService->>ItemRepo: set itemResults.status and save
else requested IN_PROGRESS/TO_RUN and current terminal
TmsService->>TmsService: mark hasRetries, buildRetryTestCaseItem
TmsService->>StepService: resetStepExecutionsForRetry (rebuild nested STEP items)
StepService->>StepExecRepo: update stepExecution records
TmsService->>ItemRepo: save retry TestItem
else active -> terminal
TmsService->>FinishHandler: finishTestItem(FinishTestItemRQ)
else terminal -> terminal
TmsService->>UpdateHandler: updateTestItem(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/epam/reportportal/base/core/tms/controller/TestCaseController.java`:
- Line 263: The long description string(s) in TestCaseController's
OpenAPI/Swagger annotation(s) exceed the 120-char limit; locate the
annotation(s) on TestCaseController that set description = "..." (the
multi-test-case update and the other two long descriptions) and split each long
literal into multiple shorter string literals concatenated with + (or into
multiple quoted segments joined by + across lines) so no source line exceeds 120
characters while preserving the exact text content and punctuation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91cadf00-d06d-4f90-8edc-bf4c38167ba4
📒 Files selected for processing (2)
src/main/java/com/epam/reportportal/base/core/tms/controller/TestCaseController.javasrc/main/java/com/epam/reportportal/base/core/tms/controller/TmsTestPlanController.java
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
2 times, most recently
from
eaddbcf to
10e70f0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java (1)
130-153: 💤 Low valueFix indentation to match project conventions.
The test method body is indented with 4 extra spaces (6 total instead of expected 4). Checkstyle flagged this as incorrect indentation level.
♻️ Fix indentation
- `@Test` - void patch_WhenStatusIsInProgress_ShouldBypassUpdateTestItemHandler() { - // Given - Long executionId = 100L; - Long launchId = 10L; - TmsTestCaseExecutionRQ request = TmsTestCaseExecutionRQ.builder().status("IN_PROGRESS").build(); - MembershipDetails membershipDetails = new MembershipDetails(); - ReportPortalUser user = mock(ReportPortalUser.class); - - when(tmsTestCaseExecutionRepository.findByTestCaseExecutionIdAndLaunchId(executionId, launchId)) - .thenReturn(Optional.of(execution1)); - when(testItemRepository.save(any(TestItem.class))).thenReturn(testItem1); - when(tmsTestCaseExecutionRepository.save(execution1)).thenReturn(execution1); - when(tmsTestCaseExecutionMapper.convert(execution1)).thenReturn(new TmsTestCaseExecutionRS()); - - // When - var result = sut.patch(membershipDetails, user, executionId, launchId, request); - - // Then - assertNotNull(result); - assertEquals(StatusEnum.IN_PROGRESS, testItem1.getItemResults().getStatus()); - verify(testItemRepository).save(testItem1); - verifyNoInteractions(updateTestItemHandler); - } + `@Test` + void patch_WhenStatusIsInProgress_ShouldBypassUpdateTestItemHandler() { + // Given + Long executionId = 100L; + Long launchId = 10L; + TmsTestCaseExecutionRQ request = TmsTestCaseExecutionRQ.builder().status("IN_PROGRESS").build(); + MembershipDetails membershipDetails = new MembershipDetails(); + ReportPortalUser user = mock(ReportPortalUser.class); + + when(tmsTestCaseExecutionRepository.findByTestCaseExecutionIdAndLaunchId(executionId, launchId)) + .thenReturn(Optional.of(execution1)); + when(testItemRepository.save(any(TestItem.class))).thenReturn(testItem1); + when(tmsTestCaseExecutionRepository.save(execution1)).thenReturn(execution1); + when(tmsTestCaseExecutionMapper.convert(execution1)).thenReturn(new TmsTestCaseExecutionRS()); + + // When + var result = sut.patch(membershipDetails, user, executionId, launchId, request); + + // Then + assertNotNull(result); + assertEquals(StatusEnum.IN_PROGRESS, testItem1.getItemResults().getStatus()); + verify(testItemRepository).save(testItem1); + verifyNoInteractions(updateTestItemHandler); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java` around lines 130 - 153, The test method patch_WhenStatusIsInProgress_ShouldBypassUpdateTestItemHandler in TmsTestCaseExecutionServiceImplTest has its method body indented 6 spaces instead of the project's 4-space standard; reformat the method body (all lines inside the method) to use 4-space indentation so checkstyle passes, preserving the existing code and assertions (including references to sut.patch, when(...) stubbings, and verify/verifyNoInteractions calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@project-properties.gradle`:
- Line 19: The migrationsUrl assignment currently hardcodes a feature branch for
non-release builds; update the logic that sets migrationsUrl (which uses
migrationsScriptsUrl and releaseMode) to use 'develop' by default for
non-release, or read an overridable project property (e.g.,
migrationsBranch/migrationsRef) when present; change the non-release branch
choice from 'feature/EMPRPP-migrate-tms-to-develop' to the property value if
supplied else 'develop' so non-release builds don't depend on a specific feature
branch.
In
`@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java`:
- Around line 155-177: The test
patch_WhenStatusIsFailed_ShouldCallUpdateTestItemHandler will NPE because
execution1 has no launchId and
tmsManualLaunchService.getTestPlanIdByLaunchId(...) is not stubbed; fix by
either setting a launchId on the execution1 test fixture used by this test or by
stubbing
tmsManualLaunchService.getTestPlanIdByLaunchId(execution1.getLaunchId()) to
return Optional.empty() or an Optional.of(testPlanId) as appropriate so
addTestCaseToTestPlan() does not call .ifPresentOrElse() on null; locate these
symbols in the test class (execution1, patch(...), addTestCaseToTestPlan,
tmsManualLaunchService.getTestPlanIdByLaunchId) and add the missing fixture
field or Mockito.when(...) stub.
---
Nitpick comments:
In
`@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java`:
- Around line 130-153: The test method
patch_WhenStatusIsInProgress_ShouldBypassUpdateTestItemHandler in
TmsTestCaseExecutionServiceImplTest has its method body indented 6 spaces
instead of the project's 4-space standard; reformat the method body (all lines
inside the method) to use 4-space indentation so checkstyle passes, preserving
the existing code and assertions (including references to sut.patch, when(...)
stubbings, and verify/verifyNoInteractions calls).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65071ffa-fae3-476c-8294-fa3e6a125312
📒 Files selected for processing (5)
project-properties.gradlesrc/main/java/com/epam/reportportal/base/core/tms/controller/TestCaseController.javasrc/main/java/com/epam/reportportal/base/core/tms/controller/TmsTestPlanController.javasrc/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.javasrc/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java
| releaseMode = project.hasProperty("releaseMode") ? project.releaseMode.toBoolean() : false | ||
| scriptsUrl = commonScriptsUrl + (releaseMode ? '5.14.0' : 'develop') | ||
| migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : 'develop') | ||
| migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : 'feature/EMPRPP-migrate-tms-to-develop') |
Contributor
There was a problem hiding this comment.
Avoid hardcoding a feature branch in shared non-release migrations URL.
Using feature/EMPRPP-migrate-tms-to-develop for all non-release builds can break builds once that branch is renamed/deleted and can desync migrations from scriptsUrl (which still tracks develop for non-release). Prefer develop (or a configurable property override) as the default non-release source.
Suggested fix
- migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : 'feature/EMPRPP-migrate-tms-to-develop')
+ def migrationsBranch = project.findProperty("migrationsBranch") ?: "develop"
+ migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : migrationsBranch)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : 'feature/EMPRPP-migrate-tms-to-develop') | |
| def migrationsBranch = project.findProperty("migrationsBranch") ?: "develop" | |
| migrationsUrl = migrationsScriptsUrl + (releaseMode ? '5.14.0' : migrationsBranch) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@project-properties.gradle` at line 19, The migrationsUrl assignment currently
hardcodes a feature branch for non-release builds; update the logic that sets
migrationsUrl (which uses migrationsScriptsUrl and releaseMode) to use 'develop'
by default for non-release, or read an overridable project property (e.g.,
migrationsBranch/migrationsRef) when present; change the non-release branch
choice from 'feature/EMPRPP-migrate-tms-to-develop' to the property value if
supplied else 'develop' so non-release builds don't depend on a specific feature
branch.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java (1)
543-564:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
request.getStatus()before branching.Line 547 now calls
StatusEnum.valueOf(...)on API input, so an unexpected status turns this patch path into anIllegalArgumentException/500 instead of a controlled domain error. Parse once withStatusEnum.fromValue(...)and reuse that enum for both the branch andaddTestCaseToTestPlan().🛠️ Suggested change
- StatusEnum targetStatus = StatusEnum.valueOf(request.getStatus().toUpperCase()); + StatusEnum targetStatus = StatusEnum.fromValue(request.getStatus()) + .orElseThrow(() -> new ReportPortalException( + "Unsupported status: " + request.getStatus() + )); if (targetStatus == StatusEnum.IN_PROGRESS || targetStatus == StatusEnum.TO_RUN) { testItem.getItemResults().setStatus(targetStatus); testItem = testItemRepository.save(testItem); execution.setTestItem(testItem); } else { execution.setTestItem( updateTestItemHandler.updateTestItem( membershipDetails, testItem, UpdateTestItemRQ.builder().status(request.getStatus()).build(), user ) ); } - addTestCaseToTestPlan(execution, request.getStatus()); + addTestCaseToTestPlan(execution, targetStatus);- private void addTestCaseToTestPlan(TmsTestCaseExecution execution, String status) { + private void addTestCaseToTestPlan(TmsTestCaseExecution execution, StatusEnum status) { if (status == null) { return; } - var statusEnum = StatusEnum.valueOf(status.toUpperCase()); - - // Check if the status is PASSED or FAILED - if (statusEnum == StatusEnum.PASSED || statusEnum == StatusEnum.FAILED) { + if (status == StatusEnum.PASSED || status == StatusEnum.FAILED) { tmsManualLaunchService .getTestPlanIdByLaunchId(execution.getLaunchId()) .ifPresentOrElse( testPlanId -> { var testCaseId = execution.getTestCaseId();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java` around lines 543 - 564, The code calls StatusEnum.valueOf(request.getStatus().toUpperCase()) which can throw IllegalArgumentException on bad input; instead parse and validate the status once using a safe converter (e.g., StatusEnum.fromValue(...) or a null/optional-returning factory) and store it in a local variable (e.g., targetStatus) before branching; use that validated targetStatus for the IN_PROGRESS/TO_RUN checks, for updateTestItemHandler.updateTestItem(...) (pass the original string or targetStatus as appropriate), and when calling addTestCaseToTestPlan(execution, ...) to avoid reparsing and to ensure controlled error handling for invalid statuses.
🧹 Nitpick comments (1)
src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java (1)
382-405: ⚡ Quick winAdd a regression test for the new delete sequence.
This change relies on deleting step executions, then the
TmsTestCaseExecution, and only then theTESTitem. Please pin that with anInOrdertest so this ordering does not drift back in a later refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java` around lines 382 - 405, Add a regression unit test for TmsTestCaseExecutionServiceImpl that verifies the deletion order using Mockito.inOrder: mock tmsStepExecutionService, tmsTestCaseExecutionRepository, testItemRepository (and testFolderItemService when suiteItemId != null), call the service method that performs the delete sequence, and assert inOrder.verify calls occur in this exact order: tmsStepExecutionService.deleteStepExecutionsByTestCaseExecution(executionId) -> tmsTestCaseExecutionRepository.deleteById(executionId) -> testItemRepository.deleteById(testItemId) (and when suiteItemId != null additionally verify testFolderItemService.deleteTestFolderTestItemByTestItemId(suiteItemId) before testItemRepository.deleteById(suiteItemId)); include at least two scenarios (with and without suiteItemId) to lock the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java`:
- Around line 543-564: The code calls
StatusEnum.valueOf(request.getStatus().toUpperCase()) which can throw
IllegalArgumentException on bad input; instead parse and validate the status
once using a safe converter (e.g., StatusEnum.fromValue(...) or a
null/optional-returning factory) and store it in a local variable (e.g.,
targetStatus) before branching; use that validated targetStatus for the
IN_PROGRESS/TO_RUN checks, for updateTestItemHandler.updateTestItem(...) (pass
the original string or targetStatus as appropriate), and when calling
addTestCaseToTestPlan(execution, ...) to avoid reparsing and to ensure
controlled error handling for invalid statuses.
---
Nitpick comments:
In
`@src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java`:
- Around line 382-405: Add a regression unit test for
TmsTestCaseExecutionServiceImpl that verifies the deletion order using
Mockito.inOrder: mock tmsStepExecutionService, tmsTestCaseExecutionRepository,
testItemRepository (and testFolderItemService when suiteItemId != null), call
the service method that performs the delete sequence, and assert inOrder.verify
calls occur in this exact order:
tmsStepExecutionService.deleteStepExecutionsByTestCaseExecution(executionId) ->
tmsTestCaseExecutionRepository.deleteById(executionId) ->
testItemRepository.deleteById(testItemId) (and when suiteItemId != null
additionally verify
testFolderItemService.deleteTestFolderTestItemByTestItemId(suiteItemId) before
testItemRepository.deleteById(suiteItemId)); include at least two scenarios
(with and without suiteItemId) to lock the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 898b3017-3896-4637-a3d6-98b944f4dc34
📒 Files selected for processing (3)
project-properties.gradlesrc/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.javasrc/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
from
10e70f0 to
9fdc72c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.java`:
- Around line 574-575: Replace usages of StatusEnum.valueOf(...) with
StatusEnum.fromValue(...) in TmsTestCaseExecutionServiceImpl to guard against
invalid status strings; specifically update the targetStatus assignments where
code currently does var targetStatus =
StatusEnum.valueOf(request.getStatus().toUpperCase()); (and the analogous
occurrence later in the same class) so they call StatusEnum.fromValue(...) and
preserve the existing error handling that maps invalid values to
ErrorType.INCORRECT_REQUEST. Locate these changes in the
TmsTestCaseExecutionServiceImpl class around the targetStatus/currentStatus
logic and ensure no other call sites in this class still use valueOf for
request-derived status strings.
In
`@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java`:
- Around line 376-389: The test uses eq(testItem1) and stubs
updateTestItemHandler.updateTestItem to return a TestItem, but the handler
actually expects a test item ID (Long) and returns an operation/result object;
update the stub and verification to use the test item ID (e.g.,
eq(testItem1.getItemId() or testItemId variable) instead of eq(testItem1) and
make the when(...).thenReturn(...) return the correct operation/result type
expected by updateTestItemHandler.updateTestItem; adjust the verify(...) call
similarly and keep sut.patch invocation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c52bd2fd-d629-4fe4-b375-6c8b9b570e76
📒 Files selected for processing (7)
project-properties.gradlesrc/main/java/com/epam/reportportal/base/core/item/TestItemService.javasrc/main/java/com/epam/reportportal/base/core/tms/mapper/NestedStepItemBuilder.javasrc/main/java/com/epam/reportportal/base/core/tms/mapper/TestCaseItemBuilder.javasrc/main/java/com/epam/reportportal/base/core/tms/service/TmsStepExecutionService.javasrc/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.javasrc/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/epam/reportportal/base/core/item/TestItemService.java
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
from
9fdc72c to
609a2b0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java (1)
224-226:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPast issue: updateTestItemHandler contract mismatch still present.
The test stubs and verifies
updateTestItemHandler.updateTestItem(...)witheq(testItem1)as the second argument, but the handler contract expects the test item ID (Long) rather than theTestItemobject itself. This was previously flagged and remains unresolved.🐛 Recommended fix from past review
when(updateTestItemHandler.updateTestItem( - eq(membershipDetails), eq(testItem1), any(UpdateTestItemRQ.class), eq(user))) - .thenReturn(testItem1); + eq(membershipDetails), eq(testItem1.getItemId()), any(UpdateTestItemRQ.class), eq(user))) + .thenReturn(null); // or appropriate operation result type // ... verify(updateTestItemHandler).updateTestItem( - eq(membershipDetails), eq(testItem1), any(UpdateTestItemRQ.class), eq(user)); + eq(membershipDetails), eq(testItem1.getItemId()), any(UpdateTestItemRQ.class), eq(user));Also applies to: 236-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java` around lines 224 - 226, The test stubs and verifies updateTestItemHandler.updateTestItem(...) using eq(testItem1) but the handler API expects the test item ID (Long); change the second argument in both the when(...) and corresponding verify(...) calls to use the test item's id (e.g., eq(testItem1.getItemId()) or the Long id variable) so the call signature matches updateTestItemHandler.updateTestItem(membershipDetails, Long, UpdateTestItemRQ, user). Ensure you update both occurrences mentioned (around the when at the shown diff and the verify at lines ~236-237) and keep the other matchers (eq(membershipDetails), any(UpdateTestItemRQ.class), eq(user)) unchanged.
🧹 Nitpick comments (2)
src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java (2)
12-14: 💤 Low valueCheckstyle: Reorder ArgumentMatchers imports.
The
ArgumentMatchersimports should appear beforeMockito.*imports per lexicographical order.📦 Suggested import order
import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java` around lines 12 - 14, Reorder the static imports so ArgumentMatchers come before Mockito imports: move the static imports for any, anyString, eq (ArgumentMatchers) to appear before any static imports from Mockito (e.g., Mockito.*) in TmsTestCaseExecutionServiceImplTest; ensure the import block follows lexicographic order by adjusting the import sequence accordingly.
154-154: 💤 Low valueCheckstyle: Test method names violate camelCase pattern.
Several test methods use snake_case naming (with underscores), which violates the checkstyle pattern
^[a-z][a-z0-9][a-zA-Z0-9]*$. While this style is descriptive and commonly used in tests, the project's checkstyle enforces strict Java camelCase conventions.Also applies to: 181-181, 212-212, 245-245, 433-433, 630-630
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java` at line 154, The test method name patch_WhenActiveStatusChangesToAnotherActive_ShouldSaveDirectlyAndBypassHandlers violates the project's camelCase Checkstyle rule; rename this method (and the other flagged test methods referenced in the comment) to conform to the pattern (e.g., patchWhenActiveStatusChangesToAnotherActiveShouldSaveDirectlyAndBypassHandlers) and update any local references or usages if present (test frameworks usually only rely on the `@Test` annotation so no other changes are typically needed); run the test suite and Checkstyle to confirm the renames resolve the violations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/epam/reportportal/base/core/tms/mapper/NestedStepItemBuilder.java`:
- Around line 85-98: The retry nested step created by buildRetryNestedStepItem
is missing propagation of the testCaseHash causing inconsistent metadata; update
buildRetryNestedStepItem to set the new TestItem's testCaseHash (e.g., call
newStep.setTestCaseHash(...)) using the existing testCaseHash from the parent
retry item (parentRetryItem.getTestCaseHash()) so retry nested steps carry the
same testCaseHash as their parent.
---
Duplicate comments:
In
`@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java`:
- Around line 224-226: The test stubs and verifies
updateTestItemHandler.updateTestItem(...) using eq(testItem1) but the handler
API expects the test item ID (Long); change the second argument in both the
when(...) and corresponding verify(...) calls to use the test item's id (e.g.,
eq(testItem1.getItemId()) or the Long id variable) so the call signature matches
updateTestItemHandler.updateTestItem(membershipDetails, Long, UpdateTestItemRQ,
user). Ensure you update both occurrences mentioned (around the when at the
shown diff and the verify at lines ~236-237) and keep the other matchers
(eq(membershipDetails), any(UpdateTestItemRQ.class), eq(user)) unchanged.
---
Nitpick comments:
In
`@src/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java`:
- Around line 12-14: Reorder the static imports so ArgumentMatchers come before
Mockito imports: move the static imports for any, anyString, eq
(ArgumentMatchers) to appear before any static imports from Mockito (e.g.,
Mockito.*) in TmsTestCaseExecutionServiceImplTest; ensure the import block
follows lexicographic order by adjusting the import sequence accordingly.
- Line 154: The test method name
patch_WhenActiveStatusChangesToAnotherActive_ShouldSaveDirectlyAndBypassHandlers
violates the project's camelCase Checkstyle rule; rename this method (and the
other flagged test methods referenced in the comment) to conform to the pattern
(e.g.,
patchWhenActiveStatusChangesToAnotherActiveShouldSaveDirectlyAndBypassHandlers)
and update any local references or usages if present (test frameworks usually
only rely on the `@Test` annotation so no other changes are typically needed); run
the test suite and Checkstyle to confirm the renames resolve the violations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fe9cb3d-68a9-4ec2-aad6-a6753bf10158
📒 Files selected for processing (7)
project-properties.gradlesrc/main/java/com/epam/reportportal/base/core/item/TestItemService.javasrc/main/java/com/epam/reportportal/base/core/tms/mapper/NestedStepItemBuilder.javasrc/main/java/com/epam/reportportal/base/core/tms/mapper/TestCaseItemBuilder.javasrc/main/java/com/epam/reportportal/base/core/tms/service/TmsStepExecutionService.javasrc/main/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImpl.javasrc/test/java/com/epam/reportportal/base/core/tms/service/TmsTestCaseExecutionServiceImplTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/epam/reportportal/base/core/item/TestItemService.java
✅ Files skipped from review due to trivial changes (1)
- project-properties.gradle
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
from
609a2b0 to
ae376f1
Compare
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
3 times, most recently
from
90e6907 to
5ca7050
Compare
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
from
5ca7050 to
f79e908
Compare
imashkanov
force-pushed
the
feature/EMPRPP-migrate-tms-to-develop
branch
from
f79e908 to
df91a6f
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Improvements
Tests
Chores