DMNCompilerImpl class code refactoring and Test cases #6267
Conversation
There was a problem hiding this comment.
Good job @ChinchuAjith
There are only minor details to fix, IMO. Thanks! 👍
| @@ -44,6 +44,7 @@ | |||
|
|
|||
| import javax.xml.namespace.QName; | |||
|
|
|||
| import org.codehaus.plexus.util.CollectionUtils; | |||
| i.getImportType()); | ||
| } | ||
| for (Import i : dmndefs.getImport()) { | ||
| if (ImportDMNResolverUtil.whichImportType(i) == ImportType.DMN) { |
There was a problem hiding this comment.
This if/else could be replaced by switch
| return ctx; | ||
| } | ||
|
|
||
| private void iterateImports(Definitions dmndefs, Collection<DMNModel> dmnModels, DMNModelImpl model, Function<String, Reader> relativeResolver ) { |
There was a problem hiding this comment.
This method need to be tested, at least verify that elements are correctly iterated over
There was a problem hiding this comment.
The if/else inside this method have been replaced with switch case. Tested and is working fine now. Please review once and let me know if any further modification required here.
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Licensed to the Apache Software Foundation (ASF) under one | |||
|
|
|||
There was a problem hiding this comment.
Pls remove this empty line
| assertThat(model).isNotNull(); | ||
| assertThat(model.getName()).isNotNull().isEqualTo(modelName); | ||
| assertThat(model.getMessages()).isNotEmpty(); |
There was a problem hiding this comment.
THis test is error prone. It rely on the assumption that there is only one message, without checking it before. Please add assertion on model.getMessages() size.
Please notice assertions on the same object could be chained
| assertThat(model).isNotNull(); | ||
| assertThat(model.getName()).isNotNull().isEqualTo(modelName); | ||
| assertThat(model.getMessages()).isNotEmpty(); |
| DMNCompilerImpl.resolveDMNImportType(input, dmnModels, model, toMerge); | ||
| assertThat(model.getMessages()).isEmpty(); | ||
| assertThat(model.getImportAliasesForNS().entrySet().stream().findFirst().get().getValue().getLocalPart()).isNotNull().isEqualTo("Imported Model"); |
There was a problem hiding this comment.
Please add an assertion on the get() invocation, or throw an exception
There was a problem hiding this comment.
Done. Added assertion on the get() invocation
| assertThat(importingModel).isNotNull(); | ||
| assertThat(importingModel.getNamespace()).isNotNull().isEqualTo(nameSpace); | ||
| assertThat(toMerge.isEmpty()).isTrue(); |
There was a problem hiding this comment.
Pls replace with more idiomatic
assertThat(toMerge).isEmpty();
| DMNCompilerImpl.checkLocatedDMNModel(input, located, model, toMerge); | ||
| assertThat(emptyNamedModel).isNotNull(); | ||
| assertThat(toMerge.isEmpty()).isFalse(); |
There was a problem hiding this comment.
Pls replace with more idiomatic
assertThat(toMerge).isNotEmpty();
kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/compiler/DMNCompilerImplTest.java
Show resolved
Hide resolved
…mpl#resolvePMMLImportType
|
@ChinchuAjith Can you please describe the changes you applied in the first message? Thanks! |
No description provided.