test(datagrid): make the JSON field's regression tests able to fail - #3059
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3056, which fixed #3051. The fix is correct. Its tests were not.
An independent review of the merged diff found three defects, all in the tests, and all of them meaning the suite could not fail if the fix were reverted.
The regression suite asserted a tautology
JsonFieldEditingModelTests.settle()drove the editor model against a realMultiRowEditState"until nothing moves" and assertedrounds == 1.JsonFieldEditingModelkeepsdisplayTextandlastSyncedbyte-identical at every exit of both mutating entry points:initassigns the same string to each,typedeither returns nil at its guard or setslastSynced = text, andreceivedendslastSynced = displayText. Somodel.typed(model.displayText)always hit the guard and returned nil, the loop always returned on its first pass,Issue.recordfor non-convergence was unreachable, and#expect(rounds == 1)held for any model, including one with the bug.Replaced with
adoptsRestatement, which asserts the invariant the fix actually rests on:MultiRowEditStatestages a compacted form of the text the editor shows, so the store always answers with a different string, and adopting that answer is the bug. That fails ifreceivedstops normalizing or loses its guard. A comment records why the loop shape was wrong so it does not come back.The UI test could never find the JSON editor
.accessibilityIdentifier("inspector-json-field")was applied toJSONCodeEditor, whose body is aSourceEditor, anNSViewControllerRepresentable. That names the representable, not theNSTextViewunderneath it. The repo had already measured this and written it down:UITestCase.editorTextViewkeeps afirstMatchfallback forsql-editor-textviewand says the identified lookup "should start working the day the identifier reaches the text view".This makes that day today.
EditorAccessibilityIdentifieris aTextViewCoordinatorthat sets the identifier on the text view fromprepareCoordinator, whichTextViewController.initcalls immediately after building it.JSONCodeEditortakes an optional identifier and holds the coordinator in@Stateso the same instance is handed over on every update.The UI test could never find the fixture table, and clicked the header
browser.staticTexts["json_fixture"]matched nothing: object browser rows are hosted cells that publish the name as the static text'svaluebehind the object kind, so the element readsTable: json_fixture.UITestCase.objectBrowserRowencodes that and five suites use it; this one hand-rolled a different lookup. The grid click also useddy: 12, which lands on the 28pt header and selects no row.Now uses
objectBrowserRow, a coordinate double-click,waitForClickableRows(which carries the non-empty-frame guard), anddy: 40.Proof that the test now constrains the fix
I reintroduced both measured root causes into the working tree and re-ran the UI test:
onChangere-reading the captured context, and the binding getter back to the per-render snapshotInspectorJsonFieldEditUITests.swift:48, "Every typed character must survive", with the editor holding the document without the typed marker (31.4s)Both defects had to go back together: with the binding getter already reading the store, re-reading the context returns the current value and the loop cannot form. That is the #3051 symptom reproduced under the test and caught by it.
Verification
buildPASS.test126/126 PASS.uitest InspectorJsonFieldEditUITests2/2 PASS, confirmed from the log rather than the count.swiftlint --strictclean on every changed file.Separately, and unrelated to this PR: the AllPlugins aggregate passes on the merged fix, and a security review of it found nothing at confidence 8 or above, including on
UITestJsonFixture, which ships in the release binary and runssqlite3_execon interpolated SQL. ItsisIsolatedgate is reachable only throughTABLEPRO_UI_TEST_SANDBOX, the SQL carries only compile-time constants, andsqlite3_open_v2is called withoutSQLITE_OPEN_CREATE.