Skip to content
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

Bug 1406515 — L10n screenshots for Photon, & ScreenGraph Evolution. #3303

Merged
merged 2 commits into from Nov 8, 2017

Conversation

@jhugman
Copy link
Contributor

@jhugman jhugman commented Oct 13, 2017

Bug 1406515 — L10n screenshots for Photon, & ScreenGraph Evolution.

This is a mega-patch that evolves ScreenGraph evolution, covering Bugs 1407710, 1407707, 1407709, 1407702.

graph

For speed of release, it also includes the L10n screenshots for Photon.

It will break the XCUITests: no attempt has been made to keep them current. Expect some amount of work post 10.0 to fix these tests.

https://bugzilla.mozilla.org/show_bug.cgi?id=1406515

@@ -273,7 +273,7 @@ class PhotonActionSheet: UIViewController, UITableViewDelegate, UITableViewDataS
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell = tableView.dequeueReusableCell(withIdentifier: PhotonActionSheetUX.CellName, for: indexPath) as! PhotonActionSheetCell
let action = actions[indexPath.section][indexPath.row]

cell.accessibilityIdentifier = action.iconString

This comment has been minimized.

@farhanpatel

farhanpatel Oct 13, 2017
Contributor

Just landed this today. You wont need to do this anymore.
c17db92

@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch to 6e96c2c Oct 18, 2017
@mozillamobilebot
Copy link

@mozillamobilebot mozillamobilebot commented Oct 18, 2017

SwiftLint found issues

Warnings

File Line Reason
SettingsTableViewController.swift 552 Force casts should be avoided.
SettingsTableViewController.swift 572 Force casts should be avoided.
PhotonActionSheet.swift 194 Force casts should be avoided.
PhotonActionSheet.swift 284 Force casts should be avoided.
PhotonActionSheet.swift 300 Force casts should be avoided.
L10nPermissionStringsSnapshotTests.swift 26 Limit vertical whitespace to a single empty line. Currently 2.

Errors

File Line Reason
PhotonActionSheet.swift 160 Opening braces should be preceded by a single space and on the same line as the declaration.
L10nPermissionStringsSnapshotTests.swift 10 There should be no space before and one after any comma.

Generated by 🚫 Danger

@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch from 0358255 to ce9b4bb Oct 19, 2017
@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch 2 times, most recently to 823db47 Nov 1, 2017
@jhugman jhugman changed the title Bug 1407710 — Upgrade ScreenGraph for L10n screenshots. Bug 1406515 — L10n screenshots for Photon, & ScreenGraph Evolution. Nov 1, 2017
Copy link
Contributor

@garvankeeley garvankeeley left a comment

Wow, this was a lot of work no doubt!

navigator.synchronizeWithUserState()
}

func restart(_ app: XCUIApplication, args: [String] = []) {

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

This backgrounds and foregrounds the app IIUC, but restart to me implies app kill and boot.

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

Yes, I agree.

I'd like it to do that: kill the app and re-launch, however, terminate and launch have the effect of effectively re-installing the app.

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

Renamed to springboardStart()

}
}
userState.url = url
navigator.performAction(Action.LoadURL)

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

new code is soo nice!

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

💯

app.tables["AppSettingsTableViewController.tableView"].cells[cellName].tap()
snapshot("04Settings-\(cellName)")
app.navigationBars.element(boundBy: 0).buttons.element(boundBy: 0).tap()
let table = app.tables.element(boundBy: 0)

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

/me c'mon Apple. there is no better function signature they could have come up with to get element by index?

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

Am thinking about a project just for XCUIElement extensions.

snapshot("10MenuOnWebPage-02")
func scroll(_ table: XCUIElement, eachScreen: (Int) -> ()) {
// TODO implement steadily scrolling up the screen
// hint: find the last cell that is hittable, and swipe up until it isn't

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

this will be great to have for the top level settings screen. I already broke this screen's l10n once by accident.

let app = XCUIApplication()
let topSites = app.cells["TopSitesCell"]
topSites.cells.matching(identifier: "TopSite").element(boundBy: 0).press(forDuration: 2.0)
navigator.openURL("http://wopr.norad.org/~sarentz/fxios/testpages/index.html")

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

this url (or string) should be a constant?

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

restart(app, args: [LaunchArguments.ClearProfile, LaunchArguments.SkipIntro])
}

func restart(_ app: XCUIApplication, args: [String] = []) {

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

Same comment as before regarding restart vs background/foreground app

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍 removed method.


func testChainedActionPerf1() {
let navigator = self.navigator!
measure {

This comment has been minimized.

@garvankeeley

garvankeeley Nov 2, 2017
Contributor

aside: I need to find out how we monitor this to indicate perf ok/bad

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch from 823db47 to 61091e5 Nov 3, 2017
@nojunpark
Copy link

@nojunpark nojunpark commented Nov 3, 2017

@jhugman would this pr include improved logging or clear method naming as we discussed it in the wiki thread? or would that be a separate PR?

@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch from 61091e5 to c221ae0 Nov 6, 2017
@jhugman
Copy link
Contributor Author

@jhugman jhugman commented Nov 6, 2017

@npark-mozilla no.

There is a plan(goto:).

transitionTo: is still to: for screen states (née scenes).

I am still not sure about the implementing: v alias:. I'd like to see how we use it (especially around more then two chained actions).

Logging can be achieved with a nodeVisitor, but this is only a fix to allow you to help yourself.

Copy link
Contributor

@justindarc justindarc left a comment

Nothing major. R+ with comments. 👍


let cancelBackAction = {
if isTablet {
// There is not Cancel option in iPad this way it is closed

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

nit: "not" -> "no"

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

screenState.tap(table.cells["TurnOnPasscode"], to: SetPasscodeScreen, if: "passcode == nil")

// screenState.tap(table.cells["TurnOffPasscode"], to: SetPasscodeScreen, if: "passcode != nil")
// screenState.tap(table.cells["ChangePasscode"], to: SetPasscodeScreen, if: "passcode != nil")

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

Remove these if they're not needed.

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

map.createScene(ClearPrivateDataSettings) { scene in
scene.backAction = navigationControllerBackAction
map.addScreenState(LoginsSettings) { screenState in
// screenState.onEnterWaitFor(element: app.tables["Login List"])

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

Remove.

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

map.addScreenState(PageOptionsMenu) {screenState in
screenState.tap(app.tables["Context Menu"].cells["menu-FindInPage"], to: FindInPage)
screenState.tap(app.tables["Context Menu"].cells["menu-Bookmark"], forAction: Action.BookmarkThreeDots, Action.Bookmark) { _ in
// NOOP

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

Is this callback block needed? If so, can we change the API to make it optional or provide a {} "NOOP" default argument?

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

self.goto(URLBarOpen)
let app = XCUIApplication()
app.textFields["address"].typeText(urlString + "\r")
openURL(urlString)

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

Why is this override method needed?

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

Followup bug for changing the tests relying on the old name. https://bugzilla.mozilla.org/show_bug.cgi?id=1414986 .

Added deprecation warning.

}

extension ScreenGraph {
fileprivate func addActionChain(_ actions: [String], finalState screenState: String?, r: @escaping UserStateChange, file: String, line: UInt) {

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

nit: r -> recorder

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

}

if current == nil {
xcTest.recordFailure(withDescription: "The app's initial state couldn't be established.",
inFile: file, atLine: line, expected: false)
}
return Navigator(self, xcTest: xcTest, initialScene: current!)
return Navigator(self, xcTest: xcTest, initialScene: current!, userState: userState)

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

This seems bad, no? We check if current is nil above, then force unwrap it anyway?

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

fatalError if no valid state can be found.

👍

let src = node.gkNode
return node.edges.values.flatMap { edge -> ConditionalEdge<T>? in
guard let predicate = edge.predicate else { return nil }
guard let dest = self.namedScenes[edge.destName]?.gkNode else { return nil }

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

Combine both guard let statements

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

}

fileprivate func leave(_ currentScene: ScreenActionNode<T>, to nextScene: GraphNode<T>, file: String, line: UInt) {
// NOP

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

nit: "NOP" -> "NOOP"

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

}

map.addScreenAction(TestActions.LoadURL, transitionTo: BrowserTab) { userState in
// NOP

This comment has been minimized.

@justindarc

justindarc Nov 6, 2017
Contributor

nit: "NOP" -> "NOOP"

This comment has been minimized.

@jhugman

jhugman Nov 6, 2017
Author Contributor

👍

@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch 2 times, most recently to 566ed77 Nov 7, 2017
jhugman added 2 commits Oct 11, 2017
…te and UserState to ScreenGraph.

#3272

[ba0457f] Bug 1407710 — Add better tidy up of the backstack.
[e420368] Bug 1407710 — Uncontroversial cleanup
[baa1fcc] Bug 1407710 — Migrate to new onEnter methods.
[bde8f5b] Bug 1407710 — Add navigator.plan()
[61709d2] Bug 1407710 — Split onEnter into onEnter and onEnterWaitFor.

This lets us add a conditional waitFor.

screenState.onEnterWaitFor(element:, if: "")
[04cc405] Bug 1407710 — Reduce visibility of test identifiers

This was causing clashes with FxScreenGraph.
[5631de4] Bug 1407710 — Add back() method, press() method, synchronizeWithUserState() method
[a2f9332] Bug 1407710 — Add extra methods to expose new actions and conditional edges to all gestures.

Improve error reporting.
[c03a87e] Bug 1407707 — Add conditional edges to the screen graph.

This lets us define edges (including actions) as conditional on the current userstate.

If the userState changes (even mid `goto`), the conditional edges are re-evaluated, and the navigator is re-routed.

Unfortunately, this commit adds mutable state to the graph, which means that there is now a one-to-one navigator-to-screengraph mapping (before you could have multiple navigators from the same instance of the ScreenGraph. This is almost certainly not worth worrying about.
[e73fc54] Bug 1407706 — More testing of actions
[9bd748e] Bug 1407706 — More testing of actions
[904c492] Bug 1407706 — Add toggleOn and toggleOff actions.

These depend on the userState being up to date.
[868d2e5] Bug 1407706 — Move enter and leave into its own private extension.
[13bdbd4] Bug 1407706 — Ensure actions always end in screen states.
[327f9b5] Bug 1407706 — Actions can be added from within screen states.
[a012a0e] Bug 1407706 — Refactor navigator.goto() into a enter and leave methods.

This also changes currentScene to be using the new generalization of GraphNode.
[059a011] Bug 1407706 — Start ScreenActionNodes

Refactor ScreenGraphNode to ScreenStateNode and GraphNode.
[6f994c0] Bug 1407709 — Add predicate tests for entry criteria into nodes.
[dc3063e] Bug 1407702 — Add onEnter user state recorders.

Unify how we look at the state of the app.
[3558577] Bug 1407702 — Add failing tests for onEnter state changes
[84cbccf] Bug 1407702 — Add UserState to ScreenGraph.

This is the basis of adding actions and predicates to the screengraph. As neither of these exist, this is not very useful.
 * Fixup NoImageTest.
 * Fixup passcode test, as it now uses 6 digits.
 * Fix launchArguments to work on both fastlane and Xcode
 * Add testPageMenuOnWebPage
 * Fixup Permissions, but not yet detecting the OK button for iOS native dialog.
 * Added test16PasscodeSettings.
 * Add toggles for hideImageMode and nightMode.
 * Fixup test24BookmarksListTableRowMenu
 * Fixup test12WebViewAuthenticationDialog, test17PasswordSnackbar, test20BookmarksTableContextMenu
 * Fixup test12WebViewAuthenticationDialog
 * Added TODO for geolocation.
 * Add accessibilityIdentifiers.
 * Enable all tests that pass, disable tests that fail.
 * fixup test18TopSitesMenu, test19HistoryTableContextMenu.
 * Fixup l10n screenshots for Photon intro.
 * Fixup screenshots to work with Photon and FxScreenGraph.
 * Add accessibility identifiers to the Photon ActionSheet (#3300)
 * Add accessibility identifiers to Photon ActionSheet.
 * Generate L10n screenshots for Photon (+2 squashed commits)
Squashed commits:
[2ea74bb] Bug 1409837 — FxScreenGraph, use new ScreenGraph facilities for Snapshotting.

 * Add extra settings screen
 * Add a forEachScreen for tables.
 * Comment use of actions.
 * Fixup FxScreenGraph to use last button as cancel button.
 * Fixup FxScreenGraph: isIpad --> isTablet.
 * Fixup FxScreenGraph for typing into the URL bar.
 * Fixup FxScreenGraph to remove troublesome URLBarAvailable and ToolbarAvailable states.
 * Add home panel context menus.
 * Fix iPad XCUITests (#3323)
 * Fixup FxScreenGraph to display the Passcode settings page and passcode interval page.
 * Fixup FxScreenGraph for nightMode and noImageMode.
 * Fixup FxScreenGraph for Settings, and Bookmarking.
 * Split the Settings Screen into two, and add TrackingProtection.
 * Add BaicAuthDialog, with backAction.
 * Add home panel context menus.
 * Move common FxUserState patterns to navigator.
 * Fixup test13ReloadButtonContextMenu
 * Fixup test11WebViewContextMenu
 * Intro, LoadingURLs, some menus, most Settings.
 * Add skeleton FxUserState to FxScreenGraph.
[7aab386] Bug 1407702 — Move navigator into BaseTestCase so XCUITests compile.
@jhugman jhugman force-pushed the jhugman/Bug1407702—add-user-state-to-screengraph branch from 566ed77 to f2b22a8 Nov 8, 2017
@jhugman jhugman merged commit 240eb59 into master Nov 8, 2017
2 checks passed
2 checks passed
Buddybuild : Fennec/57bf25c0f096bc01001e21e0 Build succeeded
Details
codecov/project No report found to compare against
Details
@jhugman jhugman deleted the jhugman/Bug1407702—add-user-state-to-screengraph branch Nov 8, 2017
jhugman added a commit that referenced this pull request Nov 13, 2017
…3303) r=justindarc

* Bug 1406515 — Generate L10n screenshots for Photon

 * Fixup NoImageTest.
 * Fixup passcode test, as it now uses 6 digits.
 * Fix launchArguments to work on both fastlane and Xcode
 * Add testPageMenuOnWebPage
 * Fixup Permissions, but not yet detecting the OK button for iOS native dialog.
 * Added test16PasscodeSettings.
 * Add toggles for hideImageMode and nightMode.
 * Fixup test24BookmarksListTableRowMenu
 * Fixup test12WebViewAuthenticationDialog, test17PasswordSnackbar, test20BookmarksTableContextMenu
 * Fixup test12WebViewAuthenticationDialog
 * Added TODO for geolocation.
 * Add accessibilityIdentifiers.
 * Enable all tests that pass, disable tests that fail.
 * fixup test18TopSitesMenu, test19HistoryTableContextMenu.
 * Fixup l10n screenshots for Photon intro.
 * Fixup screenshots to work with Photon and FxScreenGraph.
 * Add accessibility identifiers to the Photon ActionSheet (#3300)
 * Add accessibility identifiers to Photon ActionSheet.
 * Generate L10n screenshots for Photon (+2 squashed commits)
Squashed commits:
[2ea74bb] Bug 1409837 — FxScreenGraph, use new ScreenGraph facilities for Snapshotting.

 * Add extra settings screen
 * Add a forEachScreen for tables.
 * Comment use of actions.
 * Fixup FxScreenGraph to use last button as cancel button.
 * Fixup FxScreenGraph: isIpad --> isTablet.
 * Fixup FxScreenGraph for typing into the URL bar.
 * Fixup FxScreenGraph to remove troublesome URLBarAvailable and ToolbarAvailable states.
 * Add home panel context menus.
 * Fix iPad XCUITests (#3323)
 * Fixup FxScreenGraph to display the Passcode settings page and passcode interval page.
 * Fixup FxScreenGraph for nightMode and noImageMode.
 * Fixup FxScreenGraph for Settings, and Bookmarking.
 * Split the Settings Screen into two, and add TrackingProtection.
 * Add BaicAuthDialog, with backAction.
 * Add home panel context menus.
 * Move common FxUserState patterns to navigator.
 * Fixup test13ReloadButtonContextMenu
 * Fixup test11WebViewContextMenu
 * Intro, LoadingURLs, some menus, most Settings.
 * Add skeleton FxUserState to FxScreenGraph.
[7aab386] Bug 1407702 — Move navigator into BaseTestCase so XCUITests compile.

* Bug 1407710, 1407707, 1407709, 1407702 — Add ScreenActions, ScreenState and UserState to ScreenGraph.

#3272

[ba0457f] Bug 1407710 — Add better tidy up of the backstack.
[e420368] Bug 1407710 — Uncontroversial cleanup
[baa1fcc] Bug 1407710 — Migrate to new onEnter methods.
[bde8f5b] Bug 1407710 — Add navigator.plan()
[61709d2] Bug 1407710 — Split onEnter into onEnter and onEnterWaitFor.

This lets us add a conditional waitFor.

screenState.onEnterWaitFor(element:, if: "")
[04cc405] Bug 1407710 — Reduce visibility of test identifiers

This was causing clashes with FxScreenGraph.
[5631de4] Bug 1407710 — Add back() method, press() method, synchronizeWithUserState() method
[a2f9332] Bug 1407710 — Add extra methods to expose new actions and conditional edges to all gestures.

Improve error reporting.
[c03a87e] Bug 1407707 — Add conditional edges to the screen graph.

This lets us define edges (including actions) as conditional on the current userstate.

If the userState changes (even mid `goto`), the conditional edges are re-evaluated, and the navigator is re-routed.

Unfortunately, this commit adds mutable state to the graph, which means that there is now a one-to-one navigator-to-screengraph mapping (before you could have multiple navigators from the same instance of the ScreenGraph. This is almost certainly not worth worrying about.
[e73fc54] Bug 1407706 — More testing of actions
[9bd748e] Bug 1407706 — More testing of actions
[904c492] Bug 1407706 — Add toggleOn and toggleOff actions.

These depend on the userState being up to date.
[868d2e5] Bug 1407706 — Move enter and leave into its own private extension.
[13bdbd4] Bug 1407706 — Ensure actions always end in screen states.
[327f9b5] Bug 1407706 — Actions can be added from within screen states.
[a012a0e] Bug 1407706 — Refactor navigator.goto() into a enter and leave methods.

This also changes currentScene to be using the new generalization of GraphNode.
[059a011] Bug 1407706 — Start ScreenActionNodes

Refactor ScreenGraphNode to ScreenStateNode and GraphNode.
[6f994c0] Bug 1407709 — Add predicate tests for entry criteria into nodes.
[dc3063e] Bug 1407702 — Add onEnter user state recorders.

Unify how we look at the state of the app.
[3558577] Bug 1407702 — Add failing tests for onEnter state changes
[84cbccf] Bug 1407702 — Add UserState to ScreenGraph.

This is the basis of adding actions and predicates to the screengraph. As neither of these exist, this is not very useful.
jhugman added a commit that referenced this pull request Nov 14, 2017
…ution. (#3303) r=justindarc"

This reverts commit 4e81396.

Should not've been uplifted to v10.x.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants