Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace assertValues with single value to assertLastValue in pledge view model tests #2270

Merged
merged 4 commits into from
Feb 5, 2025

Conversation

amy-at-kickstarter
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter commented Jan 30, 2025

📲 What

Replace calls to assertValues([true]) to assertLastValue(true) and assertValues([false]) to assertLastValue(false).

🤔 Why

These tests frequently operate as change detectors - they break under small, trivial refactors. To whit, much of my attempts to refactor checkout code have caused harmless signal side effects which break the tests.

For example, if a signal emits [true, true] instead of [true], is it actually a bug? The UI would have no notion of that change, nor would the user. And while I wish I could make all of my refactoring purely side-effect free, it hasn't been realistic, unfortunately.

Fixing this will make it easier to refactor our checkout code, without breaking tests, and adding minimal risk.

🛠 How

Find-n-replace.

Copy link
Contributor

@jovaniks jovaniks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Replacing assertValues with assertLastValue makes sense to avoid unnecessary test failures due to minor refactors. This is especially relevant since what we actually want to verify is the last emitted state, not the entire stream. 🚀

@amy-at-kickstarter amy-at-kickstarter merged commit dbf6b11 into main Feb 5, 2025
5 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/slightly-simplify-pledge-tests branch February 5, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants