-
Notifications
You must be signed in to change notification settings - Fork 120
[Woo POS][App Listings] POS ASC screenshots #16355
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
[Woo POS][App Listings] POS ASC screenshots #16355
Conversation
Generated by 🚫 Danger |
|
|
Without bypassing eligibility checks like remote FF flag or WC versions, we’ll land in the POS ineligible case and display the error view on loading the POS tab
If we don’t handle this action in the mock, the continuation to check the firmware status never returns from `CardPresentPaymentsService.createUpdateStatePublisher`, so we’re unable to load POS fully
onChange only fires on value changes, not initial values, so when passing mocked data into the view this was only loading 10% of the times. eligibility was already .eligible and the onChange never fired
`cardReaderViewLayout` will not pass the guard despite `posModel.orderState` and `posModel.cardReaderConnectionStatus` returning the expected values if we just pass mocked products to the order, we need to pass order items as well so the `collectCardPayment` call in `PointOfSaleAggregateModel` passes the guard that checks that order items total are not zero.
…OB-1644-pos-app-listing-screenshots # Conflicts: # Modules/Sources/Yosemite/Model/Mocks/ActionHandlers/MockAppSettingsActionHandler.swift
Determines which provider to use, mock item provider for screenshot tests , or default PointOfSaleItemService
we only trigger item load when onChange value changed to be .eligible, however, if eligibility was already .eligible when view appeared (like with mocked data, as is instant), onChange never fired which would make items to never load
for some reason is ignoring `periphery:ignore:all`
No issues with this. It's the common pattern, and the mocked data probably just exposed a (slightly theoretical) bug. |
joshheald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as you mention... some suggestions inline
| if ProcessConfiguration.shouldBypassPOSOrderSyncing { | ||
| orderService = POSOrderServiceScreenshotMock(currency: currencySettings.currencyCode.rawValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't love this either.
Simple improvement – wrap it in an #if debug as well somehow?
A better (but much trickier) fix might be to set something up like the UITests use, where we mock the network rather than the service...
You could try making a factory protocol for making all this stuff, then at least we could do the ProcessConfiguration check once when creating the factory – either pass in a POSScreenshotsDependencyFactory or POSDependencyFactory – but until we have more mocked dependencies, it's probably over the top.
Best would be if we had a dependency injection framework, where we could swap these components out at will for the whole system. But Stores is the closest thing we have to that, doesn't work with async, and adding one that does is way out of scope.
Sorry, I don't have any particularly bright ideas on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing you thoughts! I'm not sure if we'll be adding further UI tests or keep them as a one-off for these ASC screenshots. For the time being I've logged this in WOOMOB-1760
Each time we run the scan it figures out a new badge of warnings from the UI test target, this should cover them all

Closes WOOMOB-1640
Description
This PR adds UI tests for POS in WooCommerceScreenshots target. These screenshots will later be used by fastlane to be uploaded to ASC. We're looking into 3 screenshots:
Feedback
I'd like specific feedback on 2 items:
onChangeonly fires on value changes and not initial values, when passing mocked data into the view this wasn't being trigger as was already available from the get go. Do you see any issue with this? I haven't, but I'm not happy changing implementation for a test.Test Steps
tools.fastlanefolder in your machine if you don't have one yet, in the path:{your machine}/Library/Caches/testScreenshots()test{your machine}/Library/Caches/tools.fastlane/screenshotsyou should see the artifacts from the screenshot testing, confirm thatpos-dashboard.png,pos-payment.png, andpos-success.pngare there.Screenshots