Account::Sync model and test fixture simplifications #968

2024-07-10T04:18:26+08:00

zachgoll commented

2024-07-10 04:18:26 +08:00

(Migrated from github.com)

This PR introduces an Account::Sync model for better visibility into the account sync process, error handling, and separation of concerns.

The two significant changes in this PR are:

Refactor account sync algorithm
Simplification of and de-coupling of account sync testing

Account sync

This PR introduces a new domain concept of Account::Sync, which acts as an audit log of individual account syncs. The sync and sync_later methods on Account are part of Account::Syncable.

In the happy path, an account sync moves from pending -> syncing -> completed, with an optional array of warnings to alert the user that something may be wrong with their account.

An unsuccessful sync transitions from pending -> syncing -> failed and populates an error specifying why the sync failed. This error is then shown on the user's account page.

Testing

Currently, we've got a ton of fixtures and have been tracking the expected family results (after account syncs) via a Google Sheet to visualize the expected results better. This was initially created as a placeholder to better visualize everything in one spot, but the codebase has started to outgrow it.

While this sheet can be very helpful as a reference, asserting against "magic numbers" in the sheet creates a tight coupling between fixtures and requires constant updates as we add in more nuance to the sync algorithm (such as the soon-to-be-implemented investment portfolios).

This PR is essentially a "reset" of fixtures and reduces each fixture file to ~2 fixtures each representing base cases. Variations of these base cases live directly in test files and test helper functions are used as our "factories" when needed. Due to the open source nature of this project, MiniTest + fixtures will likely remain the choice for testing, but these changes reflect an effort to incorporate some of the FactoryBot concepts to improve the readability of tests for newcomers.

This PR introduces an `Account::Sync` model for better visibility into the account sync process, error handling, and separation of concerns. The two significant changes in this PR are: 1. Refactor account sync algorithm 2. Simplification of and de-coupling of account sync testing ## Account sync This PR introduces a new domain concept of `Account::Sync`, which acts as an audit log of individual account syncs. The `sync` and `sync_later` methods on `Account` are part of `Account::Syncable`. In the happy path, an account sync moves from `pending` -> `syncing` -> `completed`, with an optional array of `warnings` to alert the user that something may be wrong with their account. An unsuccessful sync transitions from `pending` -> `syncing` -> `failed` and populates an `error` specifying why the sync failed. This error is then shown on the user's account page. ## Testing Currently, we've got a ton of fixtures and have been tracking the expected family results (after account syncs) via a [Google Sheet](https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit?pli=1&gid=208999873#gid=208999873) to visualize the expected results better. This was initially created as a placeholder to better visualize everything in one spot, but the codebase has started to outgrow it. While this sheet can be very helpful as a reference, asserting against "magic numbers" in the sheet creates a tight coupling between fixtures and requires constant updates as we add in more nuance to the sync algorithm (such as the soon-to-be-implemented investment portfolios). This PR is essentially a "reset" of fixtures and reduces each fixture file to ~2 fixtures each representing base cases. Variations of these base cases live directly in test files and test helper functions are used as our "factories" when needed. Due to the open source nature of this project, MiniTest + fixtures will likely remain the choice for testing, but these changes reflect an effort to incorporate some of the FactoryBot concepts to improve the readability of tests for newcomers.

zachgoll (Migrated from github.com) reviewed 2024-07-10 04:41:30 +08:00

.github/workflows/ci.yml

						
				@@ -98,4 +98,3 @@

				      - name: System tests

				        run: DISABLE_PARALLELIZATION=true bin/rails test:system

				        continue-on-error: true # TODO: Eventually we'll enforce for PRs

zachgoll (Migrated from github.com) commented

2024-07-10 04:41:30 +08:00

Codebase stability has improved, so will throw this back in.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#968