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

#3835 fluent assertions #3837

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

martin-grofcik
Copy link
Contributor

Check List:

  • Unit tests: YES
  • Documentation: NA yet

martin-grofcik and others added 6 commits February 2, 2024 21:09
…in/java/org/flowable/assertions/process/HistoricProcessInstanceAssert.java

Co-authored-by: David B Malkovsky <david@malkovsky.org>
…in/java/org/flowable/assertions/process/HistoricProcessInstanceAssert.java

Co-authored-by: David B Malkovsky <david@malkovsky.org>
…in/java/org/flowable/assertions/process/ProcessInstanceAssert.java

Co-authored-by: David B Malkovsky <david@malkovsky.org>
Copy link
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

This is an interesting idea @martin-grofcik. It allows for some good testing of processes / cases. I have some questions / ideas.

  • In my opinion I think that a single flowable-assertj module where we would add optional support for different modules is more correct. Users don't need to care which module for asserting they want to bring. They can just add flowable-assertj and use whatever they want from there. We would need to have different entry points, but that is fine.
  • I see that in the asserts we are doing DB calls all the time. What is the purpose of that? e.g. for hasVariableWithValue we first do hasVariable which will do a DB call and then we do another DB call. I think that it would be better if we treat the instance as is and don't do additional DB calls if it isn't needed.

@martin-grofcik
Copy link
Contributor Author

Hi @filiphr.

re: single flowable-assertj I will change the structure.

re: DB calls.
What I would like to achieve is:

ProcessInstanceAssert asserThatProcessInstance = FlowableProcessAssertions.assertThat(
twoTasksProcess
);
asserThatProcessInstance.isRunning()
.activities().extracting(ActivityInstance::getActivityId).containsExactlyInAnyOrder(
"theStart", "theStart-theTask", "theTask1"
);
taskService.complete(geTaskIdForProcessInstance(twoTasksProcess.getId(), taskService));
asserThatProcessInstance.isRunning().activities().extracting(ActivityInstance::getActivityId).containsExactlyInAnyOrder(
"theStart", "theStart-theTask", "theTask1", "theTask1-theTask2", "theTask2"
);
taskService.complete(geTaskIdForProcessInstance(twoTasksProcess.getId(), taskService));
asserThatProcessInstance.doesNotExist().inHistory()
.activities().extracting(HistoricActivityInstance::getActivityId).containsExactlyInAnyOrder(
"theStart", "theStart-theTask", "theTask1", "theTask1-theTask2", "theTask2", "theTask-theEnd",
"theEnd"
);

One asserThatProcessInstance evolves in the time, so there is no need to create a new instance. If we "cache" the instance status, it is not possible to achieve it.

@filiphr
Copy link
Contributor

filiphr commented Feb 15, 2024

@martin-grofcik my question is why would you like to cache the instance? Why not do something like

 asserThatProcessInstance(processInstance.getId()).isRunning() 
         .activities().extracting(ActivityInstance::getActivityId).containsExactlyInAnyOrder( 
             "theStart", "theStart-theTask", "theTask1" 
         ); 
  
 taskService.complete(geTaskIdForProcessInstance(twoTasksProcess.getId(), taskService)); 
  
asserThatProcessInstance(processInstance.getId()).isRunning().activities().extracting(ActivityInstance::getActivityId).containsExactlyInAnyOrder( 
         "theStart", "theStart-theTask", "theTask1", "theTask1-theTask2", "theTask2" 
 ); 

 taskService.complete(geTaskIdForProcessInstance(twoTasksProcess.getId(), taskService)); 
  
asserThatProcessInstance(processInstance.getId()).doesNotExist().inHistory() 
         .activities().extracting(HistoricActivityInstance::getActivityId).containsExactlyInAnyOrder( 
             "theStart", "theStart-theTask", "theTask1", "theTask1-theTask2", "theTask2", "theTask-theEnd", 
                 "theEnd" 
         ); 

Perhaps we can even have a ProcessAssert that you can instantiate and then use it via processAssert.assertThatXXX

@martin-grofcik
Copy link
Contributor Author

assertThatProcessInstance(processInstance.getId()).isRunning()

is too verbose and non fluent. I would prefer

assertThat(processInstance).isRunning()...

from the code reading perspective. assertThat(processInstance) still allows us to keep query result in the instance.

I do not know whether following adds the value:

   var assertThatOneTaskProcess = assertThat(processInstance).isRunning();

  taskService.complete(oneTaskProcessTask.getId());

  assertThatOneTaskProcess.doesNotExist();

My initial impl was storing process instance state in the assertion instance. So I do not mind.

Conclusion: What do you prefer?

@martin-grofcik
Copy link
Contributor Author

Any update?

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.

None yet

3 participants