kie-issues#1807: Test Scenario version attribute is not correctly retrieved #6254
Conversation
|
@jomarko Can you please review it? |
|
@baldimir Can you please review it? |
| "<ScenarioSimulationModel attribute2=\"whatever\" attribute1=\"whatever\" version=\"1.4\" />")); | ||
| assertThat(version).isEqualTo("1.4"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I suggest using a parameterized test here - and possibly add an explanation string, to explain the different use cases we are testing.
There was a problem hiding this comment.
Thank you @pibizza, that was my first intent, but scesim module is still based on JUnit 4, so I preferred to don't introduce Junit4 Parametrized tests in this class. About the explanation string, which approach do you suggest, please?
There was a problem hiding this comment.
@yesamer I would suggest something that describe your use case. In Junit 5 you can add a string for a specific test name. In this case add some infos in the name and restructure grouping for cases. So you can have a test where you have one entry (version or attribute), one case where you have two entries (version and attribute, times the various possible cases good or bad) and finally one case where you have three entries (version and two attributes, times the two possible values.) I expect order is not relevant, so it should not be present in the tests. at this point the name of the test could be extractXXX_version, extract XXX_version_attribute, extractXXX_version_twoattributes. You can go for different groupings, if you prefer, but that is the general idea.
| "<ScenarioSimulationModel attribute2=\"whatever\" attribute1=\"whatever\" version=\"1.4\" />")); | ||
| assertThat(version).isEqualTo("1.4"); | ||
| } | ||
|
|
There was a problem hiding this comment.
@yesamer I would suggest something that describe your use case. In Junit 5 you can add a string for a specific test name. In this case add some infos in the name and restructure grouping for cases. So you can have a test where you have one entry (version or attribute), one case where you have two entries (version and attribute, times the various possible cases good or bad) and finally one case where you have three entries (version and two attributes, times the two possible values.) I expect order is not relevant, so it should not be present in the tests. at this point the name of the test could be extractXXX_version, extract XXX_version_attribute, extractXXX_version_twoattributes. You can go for different groupings, if you prefer, but that is the general idea.
| public String extractVersion(Document document) { | ||
| try { | ||
| return document.getElementsByTagName(SCENARIO_SIMULATION_MODEL_NODE) | ||
| .item(0).getAttributes().getNamedItem(VERSION_ATTRIBUTE).getTextContent(); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("Impossible to extract version from the file", e); | ||
| } |
There was a problem hiding this comment.
My only point is only about catching an exception and immediately throwing new one. I believe we could use this approach to avoid try catch approach.
https://medium.com/@gurkanucar/how-to-prevent-null-pointer-exceptions-in-java-8d7c894c9bb4
There was a problem hiding this comment.
Thank you @jomarko, I would argue that the try-catch is present in the approach you shared, It just moved out in another method. In this case, I don't see the need for that, it would create an unnecessary indirection.
There was a problem hiding this comment.
then, I have nothing to share for this PR
Closes: apache/incubator-kie-issues#1807
The problem
The Test Scenario Engine needs to retrieve the scesim version from the xml root node to apply the correct migration strategy.
The current logic that retrieves that info, uses a regex search, that is too strict: regex patterns with this exact structure
<ScenarioSimulationModel version=\"x.x\"will be found. That means if any other attribute is present in the XML node (or just a single whitespace), the search will faileg.
<ScenarioSimulationModel version=\"x.x\"<ScenarioSimulationModel namespace="..." version=\"x.x\"The solution
In the same block of code, the DomParser of the xml file is already called. So, I just moved that in the first method line and took advantage of that to retrieve the version in a "safer" way.
This means, the regex search is no longer required, and the version can be safely retrieved disregarding its position in the node and avoiding collision with other possible attributes with the name.