refactor(web): reorganize worker-thread test helpers and test fixtures 🚂#15716
Conversation
Build-bot: skip build:web Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
Bot
changed the title
| * This file defines a unit-text fixture designed for testing | ||
| * behaviors related to the clustering of search quotient spurs | ||
| * based on just substitution/match edges. |
There was a problem hiding this comment.
The comment doesn't seem to match the content of the file.
There was a problem hiding this comment.
Any suggestions for improving this, then? That comment was intended for this file's header comment; it's not a copy-paste artifact.
There was a problem hiding this comment.
But this file contains helper methods, not the test fixture. As it is the comment would fit on buildAlphabeticClusterFixture.tests.ts IMO.
There was a problem hiding this comment.
No, that method is the test fixture - or rather, it constructs it anew on each call. It requires live construction, like the buildCantLinearFixture method.
| return node.parents.flatMap((p) => constituentPaths(p)); | ||
| } else if(node instanceof SearchQuotientSpur) { | ||
| const parentPaths = constituentPaths(node.parents[0]); | ||
| let pathsToExtend = parentPaths; |
There was a problem hiding this comment.
From devin.ai:
Unnecessary intermediate variable pathsToExtend in new constituentPaths
The new version introduces let pathsToExtend = parentPaths; then uses pathsToExtend.map(...). Since pathsToExtend is just an alias for parentPaths (same reference, never reassigned before use), this variable serves no purpose. The old code directly used parentPaths. This is not a bug — it's functionally identical — but it's a minor style inconsistency that could be cleaned up.
There was a problem hiding this comment.
It's here in part because a later PR does leverage the variable; such code appears in #15721.
| if(parentPaths.length > 0) { | ||
| return pathsToExtend.map(p => { | ||
| p.push(node); | ||
| return p; | ||
| }); |
There was a problem hiding this comment.
From devin.ai:
constituentPaths() mutates its returned arrays via push()
The constituentPaths function at web/src/test/auto/headless/engine/predictive-text/helpers/constituentPaths.ts:33-35 uses p.push(node) to mutate the arrays returned from recursive calls. This is a pre-existing pattern (carried over from the old searchQuotientUtils.ts), but it means calling constituentPaths() twice on the same node will produce incorrect results on the second call if the first call's arrays were mutated in-place by a caller. This is visible in constituentPaths.tests.ts:40-42 where the test calls constituentPaths(fixture.clusters.cluster_k3c4).map((p) => { p.push(finalPath); ... }) — this mutates the inner arrays. Since constituentPaths(finalPath) is evaluated first (as the first argument to assert.sameDeepMembers), it works, but it's fragile. The same pattern existed before, so this is not a regression.
There was a problem hiding this comment.
Recursive root = new array each time / each call. There is no reference aliasing across multiple calls of the method.
There was a problem hiding this comment.
Ah, I think we discussed this before. Didn't remember that, sorry.
|
(note my replies on some of the comments above) |
3 participants
Build-bot: skip build:web
Test-bot: skip