WIP: gci tests migration. phase 5c - #477
Draft
MatiasFernandez wants to merge 13 commits into
Draft
Conversation
expectUtf8OopToResolveViaSymbolLookup and expectUtf8OopToBeCached were private to gciLibrary.test.ts's describe. A second suite now needs the same assertion, and a copy would have dropped the reasoning the first one carries -- the looked-up session is checked with toBe rather than toHaveBeenCalledWith because koffi pointers have no enumerable properties, so vitest's deep equality treats any two sessions as equal. Both move together, along with the spyOnResolveSymbol wrapper they share, which stays private to the new module. The sessionToUse default goes away with the enclosing closure, so call sites now name the session and the library they observe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Opting a suite into commitStrategy: 'nested' opens commitDepth levels of System beginNestedTransaction before each test, so commits performed by the test (or code it exercises) land in a nested level instead of the harness's own transaction. afterEach then unwinds the whole stack — one abortTransaction per remaining level, since a single abort only steps down one level, same as a commit — and asserts the level never fell at or below the pre-nesting floor, so a test that commits past its declared budget fails loudly instead of silently corrupting the next test's session. commitDepth is modeled as a field of the 'nested' variant in a discriminated union (UseIntegrationTestOptions), not a generic useIntegrationTest param, so future commit strategies can carry their own config without polluting a shared options bag.
Fetching a Smalltalk integer via printString + Number() costs an extra encodeAsUTF8 send and paged byte fetch, and silently loses precision above 2^53 since SmallInteger is 61-bit but JS number is only exact to 2^53. oopToInteger decodes a SmallInteger or in-range LargeInteger directly off the oop via GciTsOopToI64, returning bigint to hold every value exactly. executeAndFetchInteger composes it with execute for callers that just want to evaluate code. Decoding is capped at the signed 64-bit range GciTsOopToI64 supports; LargeIntegers beyond that could still be paged out via GciTsFetchBytes if a caller ever needs it, but nothing does yet.
Drops the printString + Number() round-trip for reading System transactionLevel. Values stay bigint end-to-end, so floorLevel and the commitDepth subtraction now use BigInt instead of silently narrowing.
Its old GciTsOopToI64 call checked only err.number, ignoring the success flag the wrapper returns, so a failure could slip through as a bogus value. Delegating to the shared oopToInteger decoder closes that gap and drops the duplicated error handling.
The scenario now fits the harness's nested-transaction commit strategy: delete-history involves no instance migration, so there is no persistence question. Moves it to refactoringInstVar.committing.integration.test.ts, which runs in CI across the release matrix, and updates the gci e2e suite's header comment to describe only what's left there (migrate-instances and the two accessor-atomicity scenarios) instead of the whole set this plan already relocated.
The accessor-compile failure gates commitStructuralThenMigrate: before it runs, so migrate is never attempted and nothing commits — confirmed with a throwaway spike against a live stone. Fold it into refactoringInstVar.integration.test.ts as a plain test; no commit strategy needed, unlike its two remaining e2e siblings.
Collapse the commitStrategy: 'nested' escape hatch into a single numeric allowedCommits option, treating the default as the degenerate case of a zero-commit budget rather than a separate strategy. This removes the discriminated union, both useIntegrationTest overloads, and the branches scattered through afterEach/login/openTestTransaction, and unifies teardown so a default-strategy test that opens its own nested level no longer leaves it dangling for the next test. Also fixes an off-by-one in the public API: allowedCommits: N now means N commits, with the harness adding its own headroom level internally instead of asking authors to budget it themselves. Extract unwindAllTransactionLevels and openNestedTransactionLevels so the afterEach/beforeEach hooks read as intent rather than raw GCI doits.
That test's subject is commit semantics, so it can't use allowedCommits: a nested commit would give it a false pass. Give it a real, unarmed login of its own instead, via a local commitEmptyTransaction helper that proves the transaction empty before committing it — not a pattern to reuse for a test that needs to commit real changes.
MatiasFernandez
changed the title
An integration test that logs in on its own gets a session the harness never armed with GemStone's commit guard. Unlike a refused commit -- which fails loudly with error 2249 -- that succeeds silently and writes to the shared CI stone, and nothing detects it at runtime. Ban the three ingredients of such a session separately (a GciLibrary instance, a login call, the password), so working around one selector by renaming a receiver still trips another. The `login` selector is keyed on the receiver rather than the bare name: `testContext.login` and a destructured `login()` are the harness's own re-login and stay legitimate. Also ban the queries/forkGem import -- a forked gem is a real session the harness never armed either. Committing is deliberately not banned: the harness already refuses it in the stone, and commit literals are the legitimate subject matter of the two *.committing. files, so the rule would spend its whole budget suppressing itself. The three sanctioned sites take inline disables rather than a carve-out glob. Exempting `**/*.committing.*` would hand a login pass to any future file that merely needs a commit budget, and would exempt whole files where only one expression is sanctioned.
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
1 participant
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.
No description provided.