Enhance ACCESS_CARTA assertions in ICD tests - #91
Draft
markccchiang wants to merge 6 commits into
Draft
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.
Description
This PR addresses part of issue #3. The five ACCESS_CARTA tests cover the
REGISTER_VIEWERhandshake, but most of their assertions could not fail.protobufjsdecodes an unsetfixed32as0, an unsetstringas''and an unset map as{}, sotoBeDefined()passes on every field the backend never sets, andtoMatchObject({})matches any object. The tests were also absent from every stage file, so none of them ran in CI.This branch replaces the vacuous checks with assertions grounded in the backend source, and adds the five tests to the
file_browserstage.Keys to all five tests
session_idis asserted against what the branch inSession::OnRegisterVieweractually does, not0for a new session, the requested id for a resumed one.messagemust be non-empty and must report the session id. The backend emits a different sentence per branch, so the assertion matches on the id rather than the wording.server_feature_flagsis checked for theREAD_ONLYbit rather than compared to0, so running the backend with scripting enabled does not fail the suite.platform_stringsmust carry non-emptyrelease_info,deployment,architectureandplatformentries, andplatformmust bemacOSorLinux.user_preferencesanduser_layoutsusetoEqual({})instead oftoMatchObject({}).Per-test summary
ACCESS_CARTA_DEFAULTalso pinssession_typetoNEW, and passes theconnectTimeoutthat was declared but never used.ACCESS_CARTA_DEFAULT_CONCURRENTwas not concurrent: it awaited eachconnect()inside aforloop, so the 10 clients registered one after another. Replaced withPromise.all. Session ids are generated from a microsecond timestamp with no uniqueness check, so serial registration made the uniqueness assertion trivially true; registering in parallel now spaces the ids 11-16 microseconds apart and exercises the collision window the assertion is meant to guard. Also asserts that all 10 acknowledgements report the same server information.ACCESS_CARTA_KNOWN_SESSIONreplaces aconsole.warnguarded by an always-true condition with a real assertion on the message.ACCESS_CARTA_NO_CLIENT_FEATUREnow matches the default test check for check, which is the point of the test: the backend never readsclient_feature_flags, so no part of the response may depend on it. Also renames a test that claimed a numeric field was a non-empty string.ACCESS_CARTA_SAME_ID_TWICEdiscarded the acknowledgement returned byconnect()and asserted only the second one. Both are now captured, and their messages are asserted to report the id and to differ from each other, which is the only coverage of the reconnect branch.Checklist
For the pull request:
(or no documentation changes are needed)