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

JBPM-6487 JBPM tests stabilisation PR5 #1068

Merged
merged 4 commits into from Nov 23, 2017

Conversation

dcihak
Copy link
Contributor

@dcihak dcihak commented Nov 13, 2017

  • null check added becauseTestWorkItemHandler.getWorkItem can possibly return null,
  • null value comparison,
  • using of junit replaced by assertJ

- null check added becauseTestWorkItemHandler.getWorkItem can possibly return null,
- null value comparison,
- using of junit replaced by assertJ
@dcihak dcihak changed the title JBPM-6487: JBPM-6487 JBPM tests stabilisation Nov 13, 2017
@dcihak dcihak changed the title JBPM-6487 JBPM tests stabilisation JBPM-6487 JBPM tests stabilisation PR5 Nov 13, 2017
@dcihak
Copy link
Contributor Author

dcihak commented Nov 13, 2017

Ready to be merged

@tsurdilo
Copy link
Member

tsurdilo commented Nov 13, 2017

Whats' the reason to use verbose Assertions.assertThat everywhere and not use static import and just use "assertThat" instead ?

@MarianMacik
Copy link
Member

@dcihak can you import it statically when there is not a conflict with jUnit assertions? Thanks.

@@ -245,8 +245,7 @@ public void testSignalIntermediateThrow() throws Exception {
params.put("x", "MyValue");
ProcessInstance processInstance = ksession.startProcess(
"SignalIntermediateEvent", params);
assertEquals(ProcessInstance.STATE_COMPLETED,
processInstance.getState());
Assertions.assertThat(processInstance.getState()).isEqualTo(ProcessInstance.STATE_COMPLETED);
Copy link
Member

Choose a reason for hiding this comment

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

I think in this class it is possible.

* static import for org.assertj.core.api.Assertions added into IntermediateEventTest
* AbstractBaseTest and JbpmJUnitBaseTestCase must not extend org.junit.Assert, static import of org.junit.Assert.* added
 instead to the ancestors*
@dcihak
Copy link
Contributor Author

dcihak commented Nov 16, 2017

Static import for org.assertj.core.api.Assertions added to IntermediateEventTest, AbstractBaseTest and JbpmJUnitBaseTestCase must not extend org.junit.Assert, static import of org.junit.Assert.* added to the ancestors

@MarianMacik
Copy link
Member

jenkins retest this

1 similar comment
@MarianMacik
Copy link
Member

jenkins retest this

@dcihak
Copy link
Contributor Author

dcihak commented Nov 16, 2017

Ready to be merged.

Copy link
Member

@MarianMacik MarianMacik left a comment

Choose a reason for hiding this comment

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

@tsurdilo so basically IDE didn't recommend the static import because it would have conflicted with other assertThat methods which were in the parent Assert class. Now it is OK since we don't extend jUnit in jbpm repo.

@tsurdilo
Copy link
Member

@MarianMacik looks good now +1

@MarianMacik
Copy link
Member

I think we can merge this.

@mswiderski mswiderski merged commit 3626258 into kiegroup:master Nov 23, 2017
hxf0801 pushed a commit to hxf0801/jbpm that referenced this pull request Apr 27, 2018
* JBPM-6487:
- null check added becauseTestWorkItemHandler.getWorkItem can possibly return null,
- null value comparison,
- using of junit replaced by assertJ

* JBPM-6487:
* static import for org.assertj.core.api.Assertions added into IntermediateEventTest
* AbstractBaseTest and JbpmJUnitBaseTestCase must not extend org.junit.Assert, static import of org.junit.Assert.* added
 instead to the ancestors*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants