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.

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

[DROOLS-3368] Extend scenario runner to support DMN runtime #2194

Merged
merged 4 commits into from
Dec 19, 2018

Conversation

danielezonca
Copy link
Contributor

@danielezonca danielezonca commented Dec 12, 2018

.findFirst()
.orElseThrow(() -> new IllegalStateException("Cannot find a DMN model with resource=" + resourcePath));
}
throw new IllegalStateException("This should not happen");
Copy link
Contributor

@baldimir baldimir Dec 14, 2018

Choose a reason for hiding this comment

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

I see this was just moved here, however please change the error message here. This doesn't say anything. E.g. "Cannot retrieve the model! Resource path or model name is not set properly."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "Both resourcePath and namespace+modelName are not set, this should not happen". It is not possible that both are null, it is just to have an error if in the future the logic will change without update this method

@Before
public void init() {
registryContext = new ContextImpl();

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking (sorry:)): unnecessary empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

try {
getDMNModelCommand.execute(registryContext);
fail();
} catch (IllegalStateException ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this exception a correct state? This looks incorrect. This kind of catches in tests could hide potential bugs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrated all these catch(IllegalStateException ignored) to Assertj with a constraint on the actual message

try {
getDMNModelCommand.execute(registryContext);
fail();
} catch (IllegalStateException ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrated all these catch(IllegalStateException ignored) to Assertj with a constraint on the actual message

Copy link
Contributor

@baldimir baldimir left a comment

Choose a reason for hiding this comment

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

Thanks, approving.

Copy link

@kkufova kkufova left a comment

Choose a reason for hiding this comment

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

Looks good to me.

👍

@mariofusco mariofusco merged commit 4c68562 into apache:master Dec 19, 2018
@danielezonca danielezonca deleted the DROOLS-3368 branch June 6, 2019 09:11
tarilabs added a commit to tarilabs/drools that referenced this pull request Jun 28, 2022
* BXMSDOC-5425 Document DMN runtime listener prop

* .

* .
nprentza pushed a commit to nprentza/drools that referenced this pull request Jul 15, 2022
* BXMSDOC-5425 Document DMN runtime listener prop

* .

* .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants