Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add generic V&V reporting #2828

Closed
wants to merge 1 commit into from
Closed

add generic V&V reporting #2828

wants to merge 1 commit into from

Conversation

Rikkola
Copy link
Contributor

@Rikkola Rikkola commented Mar 20, 2020 •
edited
Loading

Main ticket:
https://issues.redhat.com/browse/DROOLS-5203

Changes to generic Validator:

  • Renaming from DRL specific objects and method to generic names
  • Modified Redundancy/Subsumption check to also cover Overlaps
  • Added the concepts of Intervals and Bounds

This adds in as experimental:

  • Overlaps
  • Misleading rows
  • Masked rows
  • Subsumptant rows
  • Range gap checks

https://github.com/kiegroup/drools/pull/2828
kiegroup/kie-wb-common#3250
kiegroup/drools-wb#1342

@Rikkola Rikkola force-pushed the DMN_VV branch 2 times, most recently from bd6ffb3 to e0ddcab Compare March 25, 2020 18:20
This was referenced Mar 26, 2020
Closed
Closed
@Rikkola
Copy link
Contributor Author

Rikkola commented Mar 30, 2020

jenkins please restest this

@Rikkola Rikkola force-pushed the DMN_VV branch 6 times, most recently from 14af236 to 776478a Compare April 6, 2020 17:14
@Rikkola
Copy link
Contributor Author

Rikkola commented Apr 7, 2020

The analysis report still has the old overlaps in it, I'm planning to remove them, but they are atm there to prove I have the same coverage.

Rikkola
Rikkola commented Apr 7, 2020
View reviewed changes
kie-dmn/kie-dmn-validation/src/main/java/org/kie/dmn/validation/dtanalysis/DMNDTAnalyser.java
@@ -141,6 +141,13 @@ private DTAnalysis dmnDTAnalysis(DMNModel model, DecisionTable dt) {
analysis.compute2ndNFViolations();
LOG.debug("computeHitPolicyRecommender");
analysis.computeHitPolicyRecommender();

Copy link
Contributor Author

@Rikkola Rikkola Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This bit below will replace the lines 124-131 above. The original overlaps, misleading and masked rows are still resolved, but not reported or used in the reports added byt addIssues below.

@jomarko jomarko self-requested a review April 8, 2020 05:57
@manstis manstis self-requested a review April 8, 2020 08:47
@Rikkola Rikkola requested a review from tarilabs April 8, 2020 09:49
@tarilabs tarilabs removed their request for review April 8, 2020 10:16
@tarilabs
Copy link
Member

tarilabs commented Apr 8, 2020

not sure about my involvement as this does not fill the agnostic API for DMN gaps and overlaps which enables any other checks. If I missed something don't hesitate to let me know

tarilabs
tarilabs reviewed Apr 8, 2020
View reviewed changes
...n-validation/src/test/java/org/kie/dmn/validation/dtanalysis/StringWithoutEnumNoGapTest.java Outdated
assertThat(analysis.getMisleadingRules(), contains(misleadingRules.toArray()));
assertTrue("It should contain DMNMessage for the MisleadingRule",
validate.stream().anyMatch(p -> p.getMessageType().equals(DMNMessageType.DECISION_TABLE_MISLEADING_RULE)));
// assertThat(analysis.getMaskedRules(), hasSize(2));
Copy link
Member

Choose a reason for hiding this comment

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

???

tarilabs
tarilabs reviewed Apr 8, 2020
View reviewed changes
.../kie-dmn-validation/src/test/java/org/kie/dmn/validation/dtanalysis/SameMsgInAllAPITest.java Outdated
assertThat(analysis.getMaskedRules(), contains(maskedRules.toArray()));
assertTrue("It should contain DMNMessage for the MaskedRule",
validate.stream().anyMatch(p -> p.getMessageType().equals(DMNMessageType.DECISION_TABLE_MASKED_RULE)));
// assertThat(analysis.getMaskedRules(), hasSize(2));
Copy link
Member

Choose a reason for hiding this comment

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

??

@Rikkola
Copy link
Contributor Author

Rikkola commented Apr 8, 2020

@tarilabs I think your find gaps implementation will stay in. However I also think both mine and your style of finding the gaps could be done better in a 3rd way.

This PR starts to remove your overlaps codes. This replaces your misleading, masked and overlapping row reporting. Overlap data will still be there since your redundancy checks use it, but both overlaps and the redundancy are on the list that I'm aiming to replace.

I marked you as a reviewer since I think you might find this interesting, but @manstis is already taking a look at the implementation. So if you are not against this then we are good.

@Rikkola Rikkola marked this pull request as ready for review April 8, 2020 10:34
@tarilabs
Copy link
Member

tarilabs commented Apr 8, 2020

Q1. some test have commented out results. If you merge this code, how is this supposed to be checked?

Q2. so are you stepping in as the maintainer for kie-dmn-validation?

Q3. How the new impl which you are putting forward will support the future DMN checks which are missing even in my impl. at the moment, please?

@Rikkola
Copy link
Contributor Author

Rikkola commented Apr 8, 2020

Answer 1.
There is still work to be done on the tests. I'll get the commented tests updated for you to prove they work, but like I mentioned in PR description old checks and and some unit tests coverage is still in. This is to prove the results match.

Answer 2.
Can answer who maintains this. I can do it, but the work here is to get in what Mark has been asking me to deliver.

Answer 3.
To answer this I could use more information on if you think these changes block future features that possibly depended on the redundancy/overlaps that you resolved? For example I'm not planning to replace all your codes, just replacing the ones that are clearly redundant. If the code you have works at the moment, I can't see a reason for me to spend yet more time replacing it. My goal is to remove feature redundancy and hopefully in the long run get some good wins for both DRL and DMN validation with this merge. Any future development can be based on my generic validator or added as a separate unit.

@Rikkola Rikkola marked this pull request as draft April 9, 2020 12:23
@Rikkola Rikkola force-pushed the DMN_VV branch 2 times, most recently from 6554f3b to 35003d5 Compare April 21, 2020 05:16
@Rikkola
Copy link
Contributor Author

Rikkola commented Apr 21, 2020

Checking impact of the changes in a PR. I expect I need to still fix few things before moving out of Draft-status.

@Rikkola
add generic V&V reporting
cdaa3c5 
DROOLS-2851 : DMN Verifier: Add support for ranges

DROOLS-2851 : DMN Verifier: Add support for ranges - bug fixes
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug C 4 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 53 Code Smells

34.2% 34.2% Coverage
3.7% 3.7% Duplication

@Rikkola Rikkola closed this Jul 1, 2020
tarilabs pushed a commit to tarilabs/drools that referenced this pull request Jun 28, 2022
@michelehaglund
BXMSDOC-6219 Changed instances of traffic-violation_1.0.0 to traffic-…
3086460 
…violation_1.0.0-SNAPSHOT (apache#161) (apache#2828)
nprentza pushed a commit to nprentza/drools that referenced this pull request Jul 15, 2022
@michelehaglund
BXMSDOC-6219 Changed instances of traffic-violation_1.0.0 to traffic-…
36c9985 
…violation_1.0.0-SNAPSHOT (apache#161) (apache#2828)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarilabs tarilabs tarilabs left review comments

@jomarko jomarko Awaiting requested review from jomarko

@manstis manstis Awaiting requested review from manstis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rikkola @tarilabs