Skip to content

Convert UI tests screens to be ScreenObject subclasses – Part 2 of many#17348

Merged
mokagio merged 13 commits intodevelopfrom
convert-to-screen-objects-p2
Oct 24, 2021
Merged

Convert UI tests screens to be ScreenObject subclasses – Part 2 of many#17348
mokagio merged 13 commits intodevelopfrom
convert-to-screen-objects-p2

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Oct 21, 2021

Continues the work started in #17221. Refer to that PR for extra context on the structure of a ScreenObject subclass.

Regression Notes

  1. Potential unintended areas of impact
    N.A.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N.A.

  3. What automated tests I added (or what prevented me from doing so)
    N.A.


  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes. N.A.
  • I have considered adding accessibility improvements for my changes. N.A.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 21, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@mokagio mokagio added this to the 18.6 milestone Oct 21, 2021
@mokagio mokagio added the Testing Unit and UI Tests and Tooling label Oct 21, 2021
@mokagio mokagio requested a review from pachlava October 21, 2021 10:37
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 21, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

let jetpackScan = mySite
.gotoJetpackScan()
let jetpackScan = try mySite
.goToJetpackScan()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice the camel case fix in the method: goto 👉 goTo. I've encountered a bunch of methods with goto and tackled them as part of the migration for the screen they belonged to.

try FancyAlertComponent().acceptAlert()
case .cancel:
FancyAlertComponent().cancelAlert()
try FancyAlertComponent().cancelAlert()
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 considered creating the FancyAlertComponent instance before the switch so that I could "reuse it" between cases. But, only one case runs at runtime, so there wouldn't have been any real benefit. Besides, the compiler might be doing other optimizations under the hood, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I've written this comment, though, I noticed that I could have done something like:

guard let alert = try? FancyAlertComponent() else { return self }

switch action {
case .accept: alert.acceptAlert()
...

🤔 I might do this in a followup...

let supportScreen = try PrologueScreen().selectContinue().selectHelp()

XCTAssert(supportScreen.isLoaded())
XCTAssert(supportScreen.isLoaded)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder: ScreenObject has this as a computed variable instead of a function.

I did this to match the Swift APIs, e.g. Array isEmpty.

@mokagio
Copy link
Contributor Author

mokagio commented Oct 21, 2021

@pachlava I run out of time in my day to see this PR through in CI.

It's possible the tests will fail due to some time outs. I think it's a mix of an issues that were there already my changes revealed and stuff I genuinely broke by changing some of the assumptions on which element is required to be available at some point in time. In my long running WIP branch, I have a few fixes that I might apply if that happens:

@mokagio mokagio force-pushed the convert-to-screen-objects-p2 branch from ecc6a2a to e15ef94 Compare October 22, 2021 02:32
That's because, before a valid email is insert, the continue button is
disabled, which fails the tests.
@mokagio mokagio marked this pull request as ready for review October 22, 2021 12:07
@mokagio mokagio enabled auto-merge October 22, 2021 12:08
public class GetStartedScreen: ScreenObject {

private let navBarGetter: (XCUIApplication) -> XCUIElement = {
$0.navigationBars["Get Started"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this was "WordPress.GetStartedView" before. I'm not sure how the tests passed, because this version can't find this view. The string also don't appear in this repo or in the authenticator pod. 🤔 Maybe the element was created but never accessed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the case.

Copy link
Contributor

@pachlava pachlava left a comment

Choose a reason for hiding this comment

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

I've executed the UI tests locally (passed), and also took a closer look into each changed file in Xcode, if it was about more than addition of throws / try - and noticed no issues. With auto-merge enabled, giving it an approval now 🚀. Thank you, Gio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Unit and UI Tests and Tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants