test(exchange): cover the Trading212 stack and add a cross-broker contract suite#1181
Open
bennycode wants to merge 2 commits into
Open
test(exchange): cover the Trading212 stack and add a cross-broker contract suite#1181bennycode wants to merge 2 commits into
bennycode wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands test coverage in packages/exchange by adding a full Trading212 test stack and a new parameterized “broker contract” suite that runs the same behavioral expectations against multiple broker implementations (currently Alpaca + Trading212), while explicitly pinning known venue divergences in one place.
Changes:
- Added unit tests for
Trading212BrokerMappercovering pending/open/filled order mapping and vendor-ticker → market-data ticker conversion. - Added integration-style (mocked API + injected market data) tests for
Trading212Broker, including order placement rules, open-order filtering, fee estimation behavior, andwatchOrderspolling behavior under fake timers. - Added a cross-broker contract test suite to enforce shared
Brokersemantics across implementations and document intentional divergences.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/exchange/src/broker/trading212/Trading212BrokerMapper.test.ts | New tests for Trading212 mapping logic (ticker normalization, pending/open/filled mapping). |
| packages/exchange/src/broker/trading212/Trading212Broker.test.ts | New tests for Trading212 broker behaviors (order placement, fees, open orders, polling order watcher). |
| packages/exchange/src/broker/trading212/api/Trading212API.test.ts | New end-to-end retry-delay calibration tests via injected axios adapter + fake timers, plus auth/baseURL checks. |
| packages/exchange/src/broker/brokerContract.test.ts | New parameterized contract suite to pin common broker semantics and explicitly assert allowed divergences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(mockMethods.getHistoryOrdersPage).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('emits "error" instead of throwing and keeps polling after an API failure', async () => { |
|
|
||
| await vi.advanceTimersByTimeAsync(1_000); | ||
| await vi.advanceTimersByTimeAsync(0); | ||
| expect(errorHandler).toHaveBeenCalledWith(new Error('Trading212 is down')); |
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.
Summary
Tests only — no production code changes. Covers the previously untested Trading212 stack and adds a cross-broker contract suite so shared
Brokersemantics (and the intentional divergences) are pinned in one place.Trading212BrokerMapper.test.ts— vendor-ticker → market-data pair conversion (AAPL_US_EQ→AAPL,BRK_B_US_EQ→BRK.B), sign-encoded quantity mapping (negative = SELL, neutralsizeunsigned), pending-order mapping, and fill mapping incl. tax-line fee summation in the account currency.Trading212Broker.test.ts(mockedTrading212API+ injectedMarketDataSource) —placeLimitOrder(DAY +extendedHours: true, never GTC),placeMarketOrder(signed quantity), notional (sizeInCounter: true) rejection,getOpenOrdersfiltering of STOP/STOP_LIMIT/VALUE orders, fee logic (0% commission,CURRENCY_CONVERSION_FEEonly on cross-currency instruments,getFeeAsset= account currency), and the pollingwatchOrdersunder fake timers: baseline snapshot (no replay of historical fills), pagination across multiple pages in a single tick,unwatchOrdersstopping the timer, and API failures emitting'error'while polling continues.Trading212API.test.ts— per-endpoint retry delays fromgetRetryDelayverified end-to-end via an injected axios adapter under fake timers (2s cash / 5s orders & portfolio / 30s account info / 50s instruments / 60s history incl. paginatednextPathURLs with query strings / 1s single-order GET / linear default for non-GET order requests), Basic-auth header construction fromapiKey:apiSecret, demo/live base-URL selection, and no-retry on HTTP 400.brokerContract.test.ts— one parameterized suite run against bothAlpacaBrokerandTrading212Broker(APIs mocked):placeLimitOrder/placeMarketOrderreturn a consistently shapedPendingOrder,cancelOpenOrdersresolves to the canceled ids,getFillsis newest-first,estimateFeecommission derives fromgetFeeRate, andwatchOrdersreturns a topicId that emits aFill. Legitimate venue divergences (Trading212 rejects notional orders; fee model & fee asset differ) are asserted explicitly in a dedicated block so this file is the single place divergences are visible.Latent findings — documented here, deliberately NOT fixed in this PR
(Parallel PRs already address the Alpaca fill-fee mapping, the Trading212
getTimecredential probe, and order-POST retry gating — this PR stays off those paths.)Trading212Broker.watchOrderscan crash the process on a transient API failure. The poll tick emits'error'on the broker'sEventEmitter; per Node semantics, an'error'event with no registered listener throws. Since it happens inside a timer callback, a consumer that never calledbroker.on('error', …)gets an uncaught exception on the first failed poll. The tests register a listener; production consumers may not.watchOrderspages the entire order history on every tick when the baseline contains no FILLED order.lastSeenIdstarts at0, and the pagination loop only stops early when it sees an id<= lastSeenId— which never happens for positive ids. Combined with the 60s-per-request rate limit on/equity/history/orders, a fill-less account makes each tick walk the full history.cancelOpenOrders/getOpenOrdersasymmetry.getOpenOrdersfilters out STOP/STOP_LIMIT and VALUE-strategy orders (no neutral representation), butcancelOpenOrderscancels every order matching the ticker, including onesgetOpenOrdersclaims don't exist. Possibly intended ("cancel everything for this pair"), but undocumented.Trading212BrokerMapper.toFilledOrdersumsMath.absof all tax lines and hardcodesposition: LONG. A positive (credit/rebate) tax entry would inflate the fee instead of netting it, and SELL fills closing a short still reportLONG(mirrors the documented Alpaca behavior).Test plan
npm testinpackages/exchange— 12 files, 147 tests passed (types + depcruise + vitest w/ coverage); 62 of those are newnpm run lintinpackages/exchange— cleanvi.mock, axios adapter injection for the retry tests); polling and retry timing viavi.useFakeTimers