Move post publish notice interaction to BlockEditorScreen #21414

tiagomar · 2023-08-24T23:40:34Z

Description

This PR moves the interaction with the post publish notice from its dedicated Screen Object into BlockEditorScreen to mitigate the risk of the notice being gone by the time the script tries to tap the View button, as discussed in #21402 (review). The notice doesn't last long in the screen and the Screen Object init takes up to a couple seconds, which is quite relevant especially running in CI. 12 out of 17 failures have the same error caused by this timing issue. The changes in this PR saves up to 3-4 seconds for the testCreateScheduledPost() between tapping the Schedule Now button and tapping the notice's View button.

Testing

CI should be 🟢

tiagomar · 2023-08-24T23:44:23Z

WordPress/UITestsFoundation/Screens/Editor/BlockEditorScreen.swift

+        guard action == .publish else { return }
+        dismissBloggingRemindersAlertIfNeeded()


Blogging Reminder is only displayed when a post is published, so we can save the 3 seconds timeout from dismissBloggingRemindersAlertIfNeeded() when scheduling a post. ⏱️

tiagomar · 2023-08-24T23:47:55Z

WordPress/UITestsFoundation/Screens/Editor/BlockEditorScreen.swift

-        if FancyAlertComponent.isLoaded() {
-            try FancyAlertComponent().acceptAlert()


I don't know which FancyAlert we could expect here. I've run all the editor tests several times and haven't seen it. We are saving some time here but it doesn't impact the target issue as it comes before tapping the Schedule Now button.

wpmobilebot · 2023-08-25T00:05:48Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21414-0af55e8
	Version	23.1
	Bundle ID	org.wordpress.alpha
	Commit	`0af55e8`
	App Center Build	WPiOS - One-Offs #6887

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-08-25T00:07:05Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21414-0af55e8
	Version	23.1
	Bundle ID	com.jetpack.alpha
	Commit	`0af55e8`
	App Center Build	jetpack-installable-builds #5927

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

jostnes · 2023-08-25T03:36:31Z

Thanks for following up on the review comment @tiagomar! The changes look fine, but I noticed that the execution time for iPad has increased quite a bit:

I'm not sure if it was just a coincidence (random slowness does happen in CI), but to be sure, I'm pushing an empty commit to this PR to make it run the tests again on iPad.

jostnes · 2023-08-25T04:26:48Z

The second (empty commit) run looks more in the normal time range, phewww! 😌

jostnes

left a suggestion to DRY the code and one really nitpicky one, other than that I think the code looks good!

jostnes · 2023-08-25T04:40:45Z

WordPress/UITestsFoundation/Screens/Editor/BlockEditorScreen.swift

+    var noticeViewButton: XCUIElement { noticeViewButtonGetter(app) }
+    var noticeViewTitle: XCUIElement { noticeViewTitleGetter(app) }


super nitpick - one of the things i'm trying to do is to sort the vars here alphabetically so it's easier to scan through the list of available elements, do you mind moving this alphabetically too? 🙏

var addBlockButton: XCUIElement { addBlockButtonGetter(app) } var applyButton: XCUIElement { applyButtonGetter(app) } var chooseFromDeviceButton: XCUIElement { chooseFromDeviceButtonGetter(app) } var closeButton: XCUIElement { closeButtonGetter(app) } var discardButton: XCUIElement { discardButtonGetter(app) } var dismissPopoverRegion: XCUIElement { dismissPopoverRegionGetter(app) } var editorCloseButton: XCUIElement { editorCloseButtonGetter(app) } var editorNavigationBar: XCUIElement { editorNavigationBarGetter(app) } var firstParagraphBlock: XCUIElement { firstParagraphBlockGetter(app) } var fullScreenImage: XCUIElement { fullScreenImageGetter(app) } var insertFromUrlButton: XCUIElement { insertFromUrlButtonGetter(app) } var keepEditingButton: XCUIElement { keepEditingButtonGetter(app) } var moreButton: XCUIElement { moreButtonGetter(app) } var noticeViewButton: XCUIElement { noticeViewButtonGetter(app) } var noticeViewTitle: XCUIElement { noticeViewTitleGetter(app) } var postSettingsButton: XCUIElement { postSettingsButtonGetter(app) } var postTitleView: XCUIElement { postTitleViewGetter(app) } var redoButton: XCUIElement { redoButtonGetter(app) } var setRemindersButton: XCUIElement { setRemindersButtonGetter(app) } var switchToHTMLModeButton: XCUIElement { switchToHTMLModeButtonGetter(app) } var undoButton: XCUIElement { undoButtonGetter(app) } var unsavedChangesLabel: XCUIElement { unsavedChangesLabelGetter(app) }

Nice! Moved these and a few others, as well as the getters, in f5e4ec4.

jostnes · 2023-08-25T05:08:05Z

WordPress/UITestsFoundation/Screens/Editor/BlockEditorScreen.swift

+    public func publish() throws {
+        return try post(action: .publish)
+    }
+
+    public func publishAndViewEpilogue() throws -> EditorPublishEpilogueScreen {
+        try publish()
+        waitAndTap(noticeViewButton)
+        return try EditorPublishEpilogueScreen()
    }

-    public func schedulePost() throws -> EditorNoticeComponent {
-        return try post(action: "Schedule")
+    public func schedulePost() throws {
+        try post(action: .schedule)
    }

-    private func post(action: String) throws -> EditorNoticeComponent {
-        let postButton = app.buttons[action]
-        let postNowButton = app.buttons["\(action) Now"]
+    public func schedulePostAndViewEpilogue() throws -> EditorPublishEpilogueScreen {
+        try schedulePost()
+        waitAndTap(noticeViewButton)
+        return try EditorPublishEpilogueScreen()
+    }


i think we can DRY this further, possibly by moving the ViewEpilogue part of the method into the post(action: postAction)? this way we won't need to create publishAndViewEpilogue() and schedulePostAndViewEpilogue(). wdyt?

something like:

public func publish() throws -> EditorPublishEpilogueScreen { return try postAndViewEpilogue(action: .publish) } public func schedulePost() throws -> EditorPublishEpilogueScreen { return try postAndViewEpilogue(action: .schedule) } private func postAndViewEpilogue(action: postAction) throws -> EditorPublishEpilogueScreen { let postButton = app.buttons[action.rawValue] let postNowButton = app.buttons["\(action.rawValue) Now"] var tries = 0 // This loop to check for Publish/Schedule Now Button is an attempt to confirm that the postButton.tap() call took effect. // The tests would fail sometimes in the pipeline with no apparent reason. repeat { postButton.tap() tries += 1 } while !postNowButton.waitForIsHittable(timeout: 3) && tries <= 3 try confirmPost(button: postNowButton, action: action) waitAndTap(noticeViewButton) return try EditorPublishEpilogueScreen() }

or if we want to keep post and view epilogue separate, we can DRY it by calling post(action: postAction) directly in publishAndViewEpilogue():

public func publishAndViewEpilogue() throws -> EditorPublishEpilogueScreen { try post(action: .publish) waitAndTap(noticeViewButton) return try EditorPublishEpilogueScreen() }

Good point, @jostnes! While working on it I decided to keep publish() in order make it possible to publish without viewing the Epilogue and maybe go to Posts or somewhere else right away? Not deeply attached to it though. WDYT?

yeah think that makes sense and very possible that we'd want to use it without viewing Epilogue so I think that's fine to keep it that way.

my second suggestion above is about publishAndViewEpilogue(), where instead of calling publish() or schedulePost, we call try post(action: PostAction) directly to remove that one extra level. not sure if it'll make a big difference in terms of timing but i think keeping the calls as direct as possible (when/if possible) usually helps with timing issues in UI tests. wdyt?

Totally! Addressed in 0af55e8.

tiagomar · 2023-08-25T13:19:56Z

And the long execution you mentioned previously #21414 (comment) cause the testCreateScheduledPost() to fail the same way despite of the improvement in this PR. 😓

While the current improvements benefit other tests, I start to think that, for testCreateScheduledPost(), maybe it's better to not rely on the Notice and change the test to confirm the post information in Posts list? WDYT?

jostnes · 2023-08-28T04:28:07Z

While the current improvements benefit other tests, I start to think that, for testCreateScheduledPost(), maybe it's better to not rely on the Notice and change the test to confirm the post information in Posts list? WDYT?

i think the same, the notice is really unpredictable in CI runs as seen for this test. i'm good with using the Posts list instead of the notice, i also did a quick check on tracks, and the number of views on the posts list is ~10x more than the epilogue views so i think in terms of coverage that could be better too (jpios_post_epilogue_view vs jpios_post_list_button_pressed).

jostnes

Thank you for considering the changes, CI is 🟢 , it's good to

Move post publish notice interaction to BlockEditorScreen

b5e5103

tiagomar added the Testing Unit and UI Tests and Tooling label Aug 24, 2023

tiagomar added this to the Someday milestone Aug 24, 2023

tiagomar requested a review from a team as a code owner August 24, 2023 23:40

tiagomar commented Aug 24, 2023

View reviewed changes

empty commit to trigger tests

8983993

jostnes reviewed Aug 25, 2023

View reviewed changes

Sort variables alphabetically

f5e4ec4

tiagomar requested a review from jostnes August 25, 2023 13:20

tiagomar added 2 commits August 28, 2023 13:37

Merge branch 'trunk' into ui-tests-improve-scheduled-post-test

fda6a2b

Verify scheduled post in post list.

0af55e8

jostnes approved these changes Aug 29, 2023

View reviewed changes

jostnes merged commit c767bb9 into trunk Aug 29, 2023

jostnes deleted the ui-tests-improve-scheduled-post-test branch August 29, 2023 02:54

		guard action == .publish else { return }
		dismissBloggingRemindersAlertIfNeeded()

		if FancyAlertComponent.isLoaded() {
		try FancyAlertComponent().acceptAlert()

		var noticeViewButton: XCUIElement { noticeViewButtonGetter(app) }
		var noticeViewTitle: XCUIElement { noticeViewTitleGetter(app) }

Move post publish notice interaction to BlockEditorScreen #21414

Move post publish notice interaction to BlockEditorScreen #21414

Uh oh!

Conversation

tiagomar commented Aug 24, 2023

Description

Testing

Uh oh!

tiagomar Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

tiagomar Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jostnes commented Aug 25, 2023

Uh oh!

jostnes commented Aug 25, 2023

Uh oh!

jostnes left a comment

Choose a reason for hiding this comment

Uh oh!

jostnes Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

tiagomar Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

jostnes Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

tiagomar Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jostnes Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

tiagomar Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

tiagomar commented Aug 25, 2023

Uh oh!

jostnes commented Aug 28, 2023

Uh oh!

jostnes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wpmobilebot commented Aug 25, 2023 •

edited

Loading

wpmobilebot commented Aug 25, 2023 •

edited

Loading

tiagomar Aug 25, 2023 •

edited

Loading