Nathan Froyd [:froydnj] Reporter
	Description • 10 years ago

With patches from bug 995417 applied:

0:27:30     INFO -  20 INFO TEST-PASS | testBookmarksPanel | Given message occurred for registered event: {"parentId":-1,"delayLoad":false,"title":"http:\/\/support.mozilla.org\/en-US\/products\/mobile","selected":false,"isPrivate":true,"stub":true,"external":false,"desktopMode":false,"tabIndex":-1,"tabID":2,"type":"Tab:Added","uri":"http:\/\/support.mozilla.org\/en-US\/products\/mobile"} - Tab:Added should equal Tab:Added
10:27:30     INFO -  EventExpecter: no longer listening for Tab:Added
10:27:30     INFO -  INFO | automation.py | Application ran for: 0:00:55.154393
10:27:30     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp7ckjG3pidlog
10:27:30     INFO -  Contents of /data/anr/traces.txt:
10:27:31     INFO -  mozcrash INFO | Downloading symbols from: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/nfroyd@mozilla.com-23ffae7aeca5/try-android/fennec-32.0a1.en-US.android-arm.crashreporter-symbols.zip
10:27:36  WARNING -  PROCESS-CRASH | testBookmarksPanel | application crashed [@ nsSocketTransport::InitiateSocket()]
...
10:27:38     INFO -  06-09 10:27:27.289 D/GeckoBrowserApp( 3517): BrowserApp.onTabChanged: 2: START
10:27:38     INFO -  06-09 10:27:27.320 I/Gecko   ( 3517): BAD CONNECT: connecting to support.mozilla.org
10:27:38     INFO -  06-09 10:27:28.429 I/WindowManager( 1402): WIN DEATH: Window{4155c8f0 org.mozilla.fennec/org.mozilla.fennec.App paused=false}

	Geoff Brown [:gbrown] Assignee
	Comment 1 • 10 years ago

In this section of testBookmarksPanel, the test navigates to bookmarks, opens the context menu on the about:firefox bookmark, and then selects Open in New Tab; then the test opens the context menu on the support bookmark and selects Open in Private Tab.

To avoid hitting the support site, this patch changes the test so that the about:firefox bookmark is used for both Open in New Tab and Open in Private Tab.

A complication is that once about:firefox is open in a tab, the bookmark displays "Switch to tab" instead of the url, causing a test failure. To get around this, I close the tab using Tabs.closeTab(tabID). (This seems like it might be useful in future, so I added a small utility function to BaseTest.)

That worked fine, but I still had slight misgivings about the potential for confusion in opening the same url twice in succession. To ease my mind, I added additional assertions to verify that not only do we get Tab:Added in both cases, but that the url in the Tab:Added message is the one we are expecting -- about:firefox.

All's well on try: https://tbpl.mozilla.org/?tree=Try&rev=e032c22b7120

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 2 • 10 years ago

Comment on attachment 8437669 [details] [diff] [review]
open about:firefox instead of support.mozilla.org

Review of attachment 8437669 [details] [diff] [review]:
-----------------------------------------------------------------

For robocop tests, I would prefer to assert state that is visible to the user rather than what's going on in the underlying implementation because they're intended to be UI tests. The underlying implementation (such as the format of the "Tab:Added" messages) can change as we refactor, but the UI should only change if we intend for it to, making it easier to know when we need to update these tests.

In this case, we can assert the page title ("About <Channel>"), close the tab, assert the page title (Robocop blank 01, or whatever), re-open the tab, assert page title ("About <Channel"), and continue the test.

Do you agree?

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 3 • 10 years ago

Comment on attachment 8437669 [details] [diff] [review]
open about:firefox instead of support.mozilla.org

Review of attachment 8437669 [details] [diff] [review]:
-----------------------------------------------------------------

In retrospect, I'm okay with this if you think it's not worth the time investment to rewrite since Tab:Added shouldn't change very much - just add a comment explaining why we're doing this (your text in comment 1 is pretty clear).

	Geoff Brown [:gbrown] Assignee
	Comment 4 • 10 years ago

I agree checking UI is generally preferred to checking events or other implementation details, at least in robocop tests.

Here, I am tempted by the Tab:Added message because it is convenient and seems like a reliable extra check that the test is proceeding as planned.

I think Tab:Added shouldn't change much, and if it does and breaks the check here, it should cause a test failure right away -- easy enough to understand and fix.

Anyway, rather than delay :froydnj's cause further, I'll accept your kind r+...

	Geoff Brown [:gbrown] Assignee
	Comment 5 • 10 years ago

With extra comments --

https://hg.mozilla.org/integration/mozilla-inbound/rev/86b272ffafec

	Ed Morley [:emorley]
	Comment 6 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/86b272ffafec

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/8415553664fd
https://hg.mozilla.org/releases/mozilla-beta/rev/5344bba7220d