test+fix: codebase audit — 48 tests, 10 emit-after-close fixes, DB testability#525
Merged
Conversation
Emit-after-close tests for 8 cubits (all fail — proving missing isClosed guards): SellConfirmCubit, SellBankAccountsCubit, SellPaymentInfoCubit, SellConverterCubit, BuyPaymentInfoCubit, TransactionHistoryMultiReceiptCubit, TransactionHistoryReceiptCubit, SettingsTaxReportCubit. Regression guards: - Non-ASCII signing (wallet_account, regression for #289) - BuyPaymentInfo equality across all fields (regression for #207) - BuyPaymentInfoCubit BitboxNotConnectedException path - DashboardBloc error survival on service failure (3 handlers) - Sell cubit: negative amount passthrough, comma normalization gap - DfxBrokerbotService: Infinity/NaN input, malformed JSON (4 methods) - BalanceService, BuyPaymentInfoService, RegistrationService: malformed JSON - SecureStorage: corrupted ciphertext (missing colon, empty string) - parseFixed edge cases (empty, multi-dot, dot, zero) - PaymentURI encoding pin, SettingsBloc hideAmounts session-only pin - WalletService persistence failure resilience (2 tests) - AppDatabase: @VisibleForTesting constructor + schema/migration tests
Adds `if (isClosed) return;` after every `await` in async methods that call `emit()`. Without these guards, navigating away from a screen while an HTTP request is in-flight throws `StateError: Cannot emit new states after calling close`. Fixed cubits: - SellConfirmCubit - SellBankAccountsCubit (add, deactivate, _loadBankAccounts) - SellPaymentInfoCubit (getPaymentInfo) - SellConverterCubit (onFiatChanged, onSharesChanged, onCurrencyChanged) - BuyPaymentInfoCubit (getPaymentInfo) - TransactionHistoryMultiReceiptCubit (generateReceipt) - TransactionHistoryReceiptCubit (generateReceipt) - SettingsTaxReportCubit (generateTaxReport)
…rterCubit TransactionHistoryFilterCubit: store the stream subscription and cancel it in close(). Previously the subscription leaked, causing potential emit-after-close on screen disposal. BuyConverterCubit: add isClosed guards in all three Timer callbacks and onCurrencyChanged (same pattern as the SellConverterCubit fix in the previous commit). Tests added: - FilterCubit: close cancels subscription (no emit after close) - BuyConverterCubit: does not emit after close - SellPaymentInfoService: malformed JSON response
Covers getUser (kyc), getTickets (support), getBalanceReport (pdf), getPortfolioHistory (account), getBankAccounts (bank account). Same pattern as the brokerbot/balance/buy/registration/sell tests from the first commit — verifies FormatException on non-JSON 200.
58178be to
25200f0
Compare
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
Implements the high-priority items from the test engineering audit (#506).
4 commits:
test: add 45 tests from codebase audit (#506)— regression guards and bug-proving testsfix: add isClosed guards to 8 cubits to prevent emit-after-close— fixes the 8 bugs those tests exposedfix: cancel subscription in FilterCubit + isClosed guards in BuyConverterCubit— 2 more cubitstest: malformed JSON response tests for remaining 5 DFX services— completes Phase 3Rebase note
This branch is 24 commits behind develop. Rebase has merge conflicts in
buy_payment_info_cubit_test.dartandsell_payment_info_cubit_test.dartbecause develop refactored those cubits (removedDFXPriceServicedependency, moved min-amount validation to API). The conflicts are in test files only — the resolution is: take develop's test structure and re-add the 3 new tests per file (BitboxNotConnected, emit-after-close, negative amount/comma).Bug fixes (10 cubits)
Added
if (isClosed) return;after everyawaitin async methods that callemit(). Without these guards, navigating away from a screen while an HTTP request is in-flight throwsStateError: Cannot emit new states after calling close.SellConfirmCubitSellBankAccountsCubitSellPaymentInfoCubitSellConverterCubitBuyPaymentInfoCubitBuyConverterCubitTransactionHistoryMultiReceiptCubitTransactionHistoryReceiptCubitSettingsTaxReportCubitTransactionHistoryFilterCubitSource change (non-behavioral)
AppDatabase.forTesting(QueryExecutor)—@visibleForTestingconstructor for in-memory DB tests. Zero production impact.Tests added (53 total)
Remaining from #506 (not in this PR)
Phase 4.1-4.3: Storage atomicity (separate PR needed)
These are real bugs but change database behavior and need careful review:
deleteWalletdoesn't cascade — only deleteswalletAccountInfos, notwalletInfos(encrypted seed persists). Already tracked as WalletStorage.deleteWallet leaves orphan walletInfos rows (encrypted seeds never deleted) #498.insertDfxTransactionnon-atomic — two separate inserts without a Drifttransaction()wrapper. If the DFX details insert fails, the Transaction row is committed as an orphan.saveBalance/saveAssetTOCTOU races — check-then-act pattern without atomicity. NeedsINSERT OR REPLACEor Drift upsert.Phase 2: Security design decisions (need discussion)
signRegistration()domain missingchainId— unlikesignDelegation()which includes it. Registration signatures are theoretically replayable across chains. Needs backend coordination.javascript:,data:,file://pass through without validation. Currently unreachable (both call sites hardcode empty amount), but should be guarded.Phase 6: Integration tests (separate scope)
5 cross-layer flows that require full DI container setup:
f9b89ea)CreateWalletState.walletis not dropped on app-hidden #489)Test plan
flutter analyzeclean on changed source filesCloses #506