test(fit): assert mxGraphView calls in the fit integration tests#3527
Conversation
Replace the smoke tests (which only checked that no error was thrown) with assertions on the scaleAndTranslate calls and the resulting scale, for every FitType and for the margin option. Mock getGraphBounds and force the container dimensions to make the scale and translation deterministic and decoupled from the parse/render pipeline. This prepares the migration to maxGraph (the mxGraph successor, with a close API) by mirroring its FitPlugin tests: configure the container dimensions, then verify the underlying mxGraphView method calls and their arguments. Pinning the exact scale and translation values lets us detect any numerical discrepancy introduced by the migration. The e2e visual tests also catch such discrepancies, on the whole computation against the real rendered bounds; these integration tests complement them with fast, precise, per-branch checks.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // even when the scale is capped to the maximum value (8). A negative margin is treated as 0 (see the 'Center' tests). | ||
| it.each` | ||
| type | dimensions | bounds | margin | expectedScale | expectedTranslateX | expectedTranslateY | ||
| ${FitType.HorizontalVertical} | ${{ offsetWidth: 480, offsetHeight: 820 }} | ${{ x: 70, y: -60, width: 100, height: 100 }} | ${undefined} | ${4.78} | ${-70} | ${60} |
There was a problem hiding this comment.
Thanks, but this is a false positive for the test data, so I'll keep the table as is.
it.each here uses a tagged template literal. Unlike a plain template string, a tagged template does not coerce its interpolated expressions: Jest's each receives the raw values and passes them to the test unchanged. The margin cell is therefore the actual undefined, not the string "undefined".
Proof from the suite itself: margin reaches ensurePositiveValue, i.e. Math.max(input ?? 0, 0). With a real undefined this returns 0, which is exactly why the ${undefined} rows assert the no-margin results (4.78 / 5.72 / 8 at translate -70, 60). If the value were the string "undefined", the expression would be Math.max(NaN, 0) → NaN, the computed scale would be NaN, and those assertions would fail. They pass, so the value is genuinely undefined.
The only undefined → string conversion happens in the generated test name ($margin → "undefined"), which is intentional: it documents the no-margin case and distinguishes those rows from the margin: 30 / margin: 100 ones. The suggestion on line 142 correctly identifies the title as the sole conversion path; the suggestions on lines 139-141 assume the data itself is converted, which it isn't.
On the proposed workarounds: each trades readability for silencing a rule that misfires on tagged-template semantics. ${null} would also change the documented input (and the Zoom table already interpolates ${null} / ${undefined} on purpose). So I'll resolve this as a false positive rather than change the code.
tbouffard
added
chore
|
1 participant
Replace the smoke tests (which only checked that no error was thrown) with assertions on the scaleAndTranslate calls and the resulting scale, for every FitType and for the margin option. Mock getGraphBounds and force the container dimensions to make the scale and translation deterministic and decoupled from the parse/render pipeline.
This prepares the migration to maxGraph (the mxGraph successor, with a close API) by mirroring its FitPlugin tests: configure the container dimensions, then verify the underlying mxGraphView method calls and their arguments. Pinning the exact scale and translation values lets us detect any numerical discrepancy introduced by the migration. The e2e visual tests also catch such discrepancies, on the whole computation against the real rendered bounds; these integration tests complement them with fast, precise, per-branch checks.
Notes
These test improvements prepare the maxGraph migration (#3238).