-
Notifications
You must be signed in to change notification settings - Fork 356
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
Update UI tests for the new WG Obfuscation views #7371
Conversation
e4a6e9e
to
ae0eba3
Compare
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.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @acb-mv)
ios/MullvadVPNUITests/SettingsMigrationTests.swift
line 142 at r2 (raw file):
UDPOverTCPObfuscationSettingsPage(app) .tapPortCell(2) .tapPortCell(1)
IMO when using indices like this in page methods it's difficult for a reader to understand what the tests do and also the tests are more prone to becoming outdated when the UI is updated. Suggest separate functions and names like tapPort80Cell
etc. Even if the separate functions for now maybe would use indices to identify elements they could in the future be improved.
ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift
line 183 at r2 (raw file):
@discardableResult func verifyUDPOverTCPPort80Selected() -> Self { let detailLabel = app.staticTexts[AccessibilityIdentifier.wireGuardObfuscationUdpOverTcpPort] XCTAssertTrue(detailLabel.label.hasSuffix("80"))
nit: this could also match other ports ending with "80" if there were any, but there isn't so probably no need to change. Or maybe it could match suffix " 80" with a leading space to avoid potential clash in the future.
2470e1e
to
db2bc11
Compare
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.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @niklasberglund)
ios/MullvadVPNUITests/SettingsMigrationTests.swift
line 142 at r2 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
IMO when using indices like this in page methods it's difficult for a reader to understand what the tests do and also the tests are more prone to becoming outdated when the UI is updated. Suggest separate functions and names like
tapPort80Cell
etc. Even if the separate functions for now maybe would use indices to identify elements they could in the future be improved.
Done
ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift
line 183 at r2 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
nit: this could also match other ports ending with "80" if there were any, but there isn't so probably no need to change. Or maybe it could match suffix " 80" with a leading space to avoid potential clash in the future.
I put in a leading space, just to be on the safe side
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.
Reviewable status: 0 of 10 files reviewed, all discussions resolved
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.
Reviewed 3 of 9 files at r1, 2 of 5 files at r2, 1 of 1 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acb-mv)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift
line 157 at r4 (raw file):
), viewModel.obfuscationUpdOverTcpPort.description) cell.setAccessibilityIdentifier(item.accessibilityIdentifier)
Do we need both a11y id:s here?
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift
line 157 at r4 (raw file):
Previously, rablador (Jon Petersson) wrote…
Do we need both a11y id:s here?
Yes. To check that the cell is selected, and to check that the detail label mentions the correct value (as in "Port: 80")
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.
Reviewed 3 of 9 files at r1, 2 of 5 files at r2, 1 of 1 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
830cffe
to
b52988e
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
This updates the UI test covering WireGuard Obfuscation settings (
SettingsMigrationTests.testChangeVPNSettings
) to work with the new SwiftUI-based pages, traversing both the UDP/TCP and Shadowsocks settings pages, and in turn implements UI test support forSingleChoiceList
-based pages. This does not implement any new tests.This change is