Fix TR-5932 branching rules evaluation for non-linear test parts #381
Conversation
| if ( | ||
| $ignoreBranchings === false && | ||
| $numberOfBranchRules > 0 && | ||
| $this->mustApplyBranchRules() || $branchRules->isNonLinearNavigationModeAllowed() |
There was a problem hiding this comment.
Just a sanity check.
Did you mean what you've written, or did you mean this instead?
$ignoreBranchings === false &&
$numberOfBranchRules > 0 &&
($this->mustApplyBranchRules() || $branchRules->isNonLinearNavigationModeAllowed())
There was a problem hiding this comment.
In either case, I strongly advise using parenthesis (even if the order of logical operations is correct) to emphasize the intended logic.
There was a problem hiding this comment.
I agree with you, let me improve that
There was a problem hiding this comment.
Oh. So, it was a wrong condition after all?
I mean, what you've applied now is totally different from the original condition where, should $branchRules->isNonLinearNavigationModeAllowed() be true the rest of the condition didn't matter.
There was a problem hiding this comment.
Now the question is: "why didn't the test cases fail with the original code"?
There was a problem hiding this comment.
We probably do not have proper coverage for those conditions. But this is quite a PISA emergency, I am not sure I can cover all the previous missing cases on this PR.
There was a problem hiding this comment.
That's fine. Just once again want to be 100%-sure your changes are intentional.
Quick reference of the changes between the two conditions we saw in this PR before and after my comment on this line: https://3v4l.org/G5cLN
There was a problem hiding this comment.
Yes, it is intentional, before the || operator outside of the parentheses would invalidate all the time all the other rules (which was passing cause of low test coverage), but it only makes sense to pass if other conditions are met too :)
Thanks for the heads up
There was a problem hiding this comment.
OK then.
I just didn't realize that from your initial response to my comment.
All clear now.
There was a problem hiding this comment.
FYI, $ignoreBranchings and other parameters for this very method are never passed. They were introduced on 2020, but never used. I will not change them now to avoid inflate the PR, but that is also one of the reasons that condition is not caught on tests
wazelin
changed the title
gabrielfs7
deleted the
fix/TR-5932/fix-branching-rules-nonlinear-test-parts
branch
https://oat-sa.atlassian.net/browse/TR-5932
Make branch rules work again between test part with nonlinear navigation
Related PR