Skip to content

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Nov 13, 2025

Found this one while trying to generate new screenshots for ASC

Description

WooCommerceScreenshots target crashes when running testScreenshots() due to unhandled actions that are dispatched through MockActionHandler

One review is enough. No rush to review/merge into 23.7, it's an internal crash only when we run the target, which doesn't run in CI 👍

Test Steps

  • Switch to WooCommerceScreenshots
  • Run WooCommerceScreenshots.testScreenshots()
  • App should not crash. Test should pass*

* The first time I ran the tests all of them failed due to a Tap to Pay modal appearing in the middle of the screen, which wasn't dismissed automatically so tests couldn't navigate through the app to make screenshots. A second run worked fine. I'm guessing because we only show it once, but I'm unaware of the exact details at this point. If tests fail due to this reason please run them again, we can handle the tap to pay modal separately.

Simulator Screenshot - iPad (A16) - Tests - 2025-11-13 at 15 38 55

@iamgabrielma iamgabrielma added this to the 23.7 milestone Nov 13, 2025
@iamgabrielma iamgabrielma added type: task An internally driven task. type: technical debt Represents or solves tech debt of the project. Bug labels Nov 13, 2025
@wpmobilebot
Copy link
Collaborator

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16352-6062f08
Version23.6
Bundle IDcom.automattic.alpha.woocommerce
Commit6062f08
Installation URL1fbmds03c74ko
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@iamgabrielma iamgabrielma modified the milestones: 23.7, 23.8 Nov 14, 2025
@joshheald joshheald self-assigned this Nov 17, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

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

Works well, one question

.getPOSSurveyPotentialMerchantNotificationScheduled,
.getHasPOSBeenOpenedAtLeastOnce:
break
default: unimplementedAction(action: action)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Should we actually have this default, if this is causing issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in the middle about it:

I think we shouldn't use default in general to just hide from the compiler that the action hasn't been handled, despite being a mock it just forces a runtime error for no reason imho.

On the other side is not causing much trouble since only runs when we have to trigger the screenshot tests. I had to add a couple more actions here on a separate PR while working with WooCommerceScreenshots but that's about it. Updating it now means adding 67 actions to the switch statement that are missing 😅

@iamgabrielma iamgabrielma merged commit 8717ac1 into trunk Nov 18, 2025
29 of 32 checks passed
@iamgabrielma iamgabrielma deleted the task/fix-crash-screenshots-unhandled-action-dispatch branch November 18, 2025 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug type: task An internally driven task. type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants