SD-1823 Revisit the OVS checks with unit test cases #235
Conversation
This file contains 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
CI Failure Feedback 🧐(Checks updated until commit a75e892)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
jkosternl
changed the title
…alse and removed TODO
…ant changes to OvsChecksTest.java
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Code Suggestions ✨Explore these optional code suggestions:
|
…validation and change the limit to 5.
jkosternl
reviewed
Nov 29, 2024
ovs/src/main/java/org/dcsa/conformance/standards/ovs/party/OvsPublisher.java
Outdated
Show resolved
Hide resolved
| sspSupplier, | ||
| OvsFilterParameter.FACILITY_SMDG_CODE, | ||
| "*/vesselSchedules/*/transportCalls/*/location/facilitySMDGCode"))); | ||
| "If present, at least schedule attribute must match the corresponding query parameters", |
There was a problem hiding this comment.
Does this mean at least one schedule? I didn't get it from this message string
There was a problem hiding this comment.
it should have been "at least one schedule". it corrrect it
ovs/src/main/java/org/dcsa/conformance/standards/ovs/party/OvsFilterParameter.java
Show resolved
Hide resolved
| if (RETURN_EMPTY_RESPONSE) { | ||
| return request.createResponse( | ||
| 200, headers, new ConformanceMessageBody(OBJECT_MAPPER.createArrayNode())); | ||
| } else if (USE_WRONG_ATTRIBUTE_VALUES) { |
There was a problem hiding this comment.
Do we need two different responses for both wrong attributes and dates?
…ded unit test cases.
jkosternl
requested changes
Dec 4, 2024
ovs/src/main/java/org/dcsa/conformance/standards/ovs/checks/OvsChecks.java
Outdated
Show resolved
Hide resolved
ovs/src/main/java/org/dcsa/conformance/standards/ovs/checks/OvsChecks.java
Show resolved
Hide resolved
ovs/src/test/java/org/dcsa/conformance/standards/ovs/OvsChecksTest.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| return Set.of(); | ||
| })); | ||
|
|
||
| return JsonAttribute.contentChecks( |
There was a problem hiding this comment.
@preetamnpr, I've checked on how you can unit test this without the modifications in JsonAttributeBasedCheck. I think it is possible if you move each separate check into a package private method, which generates just one check. Or if you don't like that, just have this method return the checks and make it package private. The checks itself are easy to validate with. And create the wrapper a level higher: JsonAttribute.contentChecks(OvsRole::isPublisher, matched, HttpMessageType.RESPONSE, standardVersion, checks);
There was a problem hiding this comment.
The wrapper method works and this doesnt require any core changes in order to build the unit test cases. I have implemented the wrapper method and it works fine with no changs to core.
There was a problem hiding this comment.
It was just a suggestion on how you can more easily unit test it. The last sentence on the wrapper is just a side note.. just keep it if it is needed. No worries.
| .map(Map.Entry::getKey) | ||
| .collect(Collectors.joining(", ")), // Get keys here | ||
| attributeValues.size() > 1 ? "" : "es", | ||
| parameterValues.size() > 1 ? "s" : "", |
There was a problem hiding this comment.
You're doing this very properly..but I think perhaps it is too nice 😉 . You can just safe time and code by doing this: Value(s). If it was on the front page of a site, it is important, but since this is an error code, which is still clear, you can make it yourself easy.
ovs/src/test/java/org/dcsa/conformance/standards/ovs/OvsChecksTest.java
Outdated
Show resolved
Hide resolved
ovs/src/test/java/org/dcsa/conformance/standards/ovs/OvsChecksTest.java
Outdated
Show resolved
Hide resolved
ovs/src/test/java/org/dcsa/conformance/standards/ovs/OvsChecksTest.java
Outdated
Show resolved
Hide resolved
jkosternl
approved these changes
Dec 4, 2024
| "CheckServiceSchedules failed: %s".formatted(validationError))); | ||
| if (validationErrors.isEmpty()) { | ||
| return Set.of(); | ||
| } |
There was a problem hiding this comment.
Line 41-43 can be removed, because line 44 does the same (return empty Set, or with entries).
|
|
||
| if (body == null || body.isMissingNode() || body.isNull()) { | ||
| return Set.of("Response body is missing or null."); | ||
| } else { |
There was a problem hiding this comment.
Suggested change
| } else { | |
| } |
…on to return empty set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
User description
I am enhancing the unit test cases to capture all possible cases. Once its done this PR will be ready for the review
PR Type
Enhancement, Tests
Description
Changes walkthrough 📝
6 files
OvsScenarioListBuilder.java
Update scenario dates for consistencyovs/src/main/java/org/dcsa/conformance/standards/ovs/OvsScenarioListBuilder.java
OvsChecks.java
Refactor and enhance OVS checks with loggingovs/src/main/java/org/dcsa/conformance/standards/ovs/checks/OvsChecks.java
@Slf4j.OvsFilterParameter.java
Enhance OvsFilterParameter with additional attributesovs/src/main/java/org/dcsa/conformance/standards/ovs/party/OvsFilterParameter.java
@Getterannotation.OvsPublisher.java
Add test flags for response scenarios in OvsPublisherovs/src/main/java/org/dcsa/conformance/standards/ovs/party/OvsPublisher.java
ovs-300-response.json
Update transport call reference and event date timeovs/src/main/resources/standards/ovs/messages/ovs-300-response.json
OVS_v3.0.0.yaml
Restrict additional properties in UNLocationLocation schemaovs/src/main/resources/standards/ovs/schemas/OVS_v3.0.0.yaml
additionalPropertiesto false for UNLocationLocation.4 files
OvsChecksTest.java
Add unit tests for OvsChecks validation logicovs/src/test/java/org/dcsa/conformance/standards/ovs/OvsChecksTest.java
ovs-300-response-wrong-attribute-values.json
Add test message with incorrect attribute valuesovs/src/main/resources/standards/ovs/messages/ovs-300-response-wrong-attribute-values.json
ovs-300-response-wrong-date-times.json
Add test message with incorrect date formatsovs/src/main/resources/standards/ovs/messages/ovs-300-response-wrong-date-times.json
ovs-300-response-wrong-structure.json
Add test message with incorrect structureovs/src/main/resources/standards/ovs/messages/ovs-300-response-wrong-structure.json
1 files
pom.xml
Add JUnit Jupiter dependency for testingovs/pom.xml