Test and document HumanTaskAssert

[margue]

write tests to assure API stability and document

[margue]

While writing systematic tests for HumanTaskAssert yesterday, I came across an issue.
Testing the isCompleted method would mean to write

assertThat(caseInstance).humanTask(TASK_A).isCompleted();

as assertion. However, this results in an AssertionError: Expecting actual not to be null. This is because a completed task cannot be retrieved via CaseExecutionQuery, it is only accessible via HistoricCaseActivityInstanceQuery.

In detail: assertThat(caseInstance).humanTask(TASK_A) fails, because CaseExecutionQuery responded with null. This means, the method isCompleted in HumanTaskAssert is only working, if the Task is not completed (and not disabled (maybe same for failed and/or suspended, did not try it yet)).

The hasState(CaseExecutionState) method in AbstractCaseAssert uses both ways to verify the state of a CaseExecution. This means, that

assertThat(taskA).isCompleted();

works in both situations correctly (but is not part of this issue).

As having a method than can be called only if its outcome is false, feels somehow strange and could lead to confusion for users of the framework, I see two options to handle this:

1. Add proper exception handling, so that
   assertThat(caseInstance).humanTask(TASK_A)
   delivers at least a message, why this does not work on completed (disabled/...) Tasks.
2. Adapt the behavior of hasState(CaseExecutionState) to humanTask(CaseExecutionQuery) (and for all other PlanItemTypes) using HistoricCaseActivityInstanceQuery.

As the second option on first sight seems more comfortable to me, I worry about hiding insights into the camunda case engine, that a developer of a case should probably be aware of.

Any comments? Thanks!

[martinschimak]

Hi @margue - first of all: many thanks for contributing work and thoughts on that.

May I ask you to read the end of comment #52 (comment) first. Under Point 6 I basically discuss this issue. As I did not get any feedback on this, I already decided to move in the direction I propose there and changed your old cmmn code accordingly.

Because of what I say there, with respect to your options, I propose to go for option 1 to eventually think about improving messages when checking isCompleted() on null references. The main issue for such a message is more that the behaviour/contract of the chained method humanTask(TASK_A) must be very well understood by developers: it always looks at runtime tables. It is therefore a legitimate result that it returns a null from runtime tables. so e.g. checking for assertThat(caseInstance).humanTask(TASK_A).isNull() would of course work as expected.

The basic principles for camunda-bpm-assert always were:

1. Retrieve a reference for a runtime object, let's say a task, e.g. by using helper methods. Helper methods always look at runtime tables only.

2. As long as you hold that reference in your test code, you can use every assertion on it - including completed(). Therefore àssertThat(taskA).isCompleted() does work: Assertions potentially look at history tables, too, to retrieve details about the object under test, e.g. because it does not exist as a runtime object anymore, or because it does exist, but the assertion is designed to look at the current history of the object under test.

Consequence: hasState() is an assertion (therefore potentially using history tables to retrieve details about the object under test) while humanTask(...) is a helper method designed to retrieve a reference (and wrap it into an assertion).

I agree that the situation is a bit tricky to grasp first for relatively new users of camunda-bpm-assert, when it comes to the "chained" methods like humanTask(...), but I am confident that it makes sense as a consistent implementation stratey: 1) All helper methods (already used by the community) work like that 2) Let's also not forget that the history tables can contain several objects with the same key.

However, any (critical) thoughts are always welcome of course!

[malteser]

Hi,

I, personally, would go for option 2 as it is more intuitive from a new user's POV, but as this would break the principles of this project, we have to go for number 1.

Let's document the behaviours ("looks at runtime only", "looks at both runtime and history") and add a new layer of convenience functions on top of this framework.

[margue]

Hi @martinschimak - thanks for your fast and detailed answer. And sorry for not having read #52 issues comment beforehand.

I agree, that the cmmn part should be consistent to the bpmn part. So I just started writing some tests following the first option.