Closed Bug 1395409 Opened 7 years ago Closed 7 years ago

Write test for Activity Stream Pocket referrer URI

Categories

(Firefox for Android Graveyard :: Activity Stream, defect, P2)

Product:
Firefox for Android Graveyard
Component:
Activity Stream
Platform:
All
Android
Type:
defect

Tracking

(fennec+, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
fennec + ---
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Whiteboard: [mobileAS])

Attachments

(1 file, 1 obsolete file)

Bug 1395409: Add testActivityStreamPocketReferrer.
59 bytes, text/x-review-board-request
liuche
: review+
Details
afaik, the referrer URIs we send when a user clicks on Pocket link are really important to Pocket. (In reply to Nate Weiner from bug 1380808 comment #31) > That link is less about tracking and more about making sure the publisher > sees a referer for the link. (Otherwise publishers have no idea we're > sending them traffic). Since it's so important, we should write a test to ensure this support never regresses. This should be a UI test where we click (& open in new tab) on the Pocket AS items and verify the referrer URI is passed. Then we should click on other AS items and verify the referrer URI is not passed. A unit test wouldn't be sufficient because we could always change where the code is called from To verify the referrer URI is passed, ideally we'd get the request on a server and verify the received headers. However, considering that's extremely challenging, if not impossible, in our harness, I think the ideal would be to intercept the "Tab:Load" JS event and verify that referrerURI is included in the data. --- Adding tracking=? because this is a regression test (it was already tested manually) so it doesn't seem as immediately important as fixing other AS bugs before 57. Keeping assigned because I have most of it written - I'm just struggling to capture the JS events.
Jim, EventDispatcher is undefined in my code - do you know why? I was trying to follow testEventDispatcher but EventDispatcher does not seemed undefined there, for whatever reason.
Flags: needinfo?(nchen)
Yeah it's a little confusing. `EventDispatcher` is defined inside Messaging.jsm, which testEventDispatcher imports [1]. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/tests/browser/robocop/testEventDispatcher.js#1
Flags: needinfo?(nchen)
Jim, my listener doesn't seem to get the Tab:Load event - any ideas?
Flags: needinfo?(nchen)
Tab:Load goes through the GeckoApp event dispatcher, which you can access through GeckoApp.getAppEventDispatcher [1]. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testEventDispatcher.java#73
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #6) > Tab:Load goes through the GeckoApp event dispatcher, which you can access > through GeckoApp.getAppEventDispatcher [1]. If I understand correctly, I think you mean that I can listen for the Tab:Load event in Java by accessing the getAppEventDispatcher. However, it looks like `dispatch` calls directly into Gecko (and thus I assume JS) [1]. Anecdotally, I added a listener to getAppEventDispatcher in Java and it doesn't get called for Tab:Load events. I see a possibility where I can override the EventDispatcher in GeckoApp with a mock to capture any Tab:Load events but that seems more hacky than adding an additional listener for the Tab:Load event in JS. Is that possible to do, Jim? Or am I misunderstanding something? [1]: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java#242
Flags: needinfo?(nchen)
Ah right you're listening to a Gecko thread event, so that explains why your Java listener doesn't receive it. It's strange though why your JS listener is not receiving the event either. If I add the following to browser.js, > let win = Services.wm.getMostRecentWindow("navigator:browser"); > EventDispatcher.for(win).registerListener((event, data, callback) => > dump("Load: " + JSON.stringify(data)), ["Tab:Load"]); I do get Tab:Load events in the log, > 12745 12798 D GeckoBrowser: Load: {"userEntered":false,"engine":null,"newTab":true,"pinned":false,"isPrivate":false,"delayLoad":false,"url":"about:home","tabID":1,"parentId":-1,"selected":true,"desktopMode":false} > 12745 12798 D GeckoBrowser: Load: {"userEntered":false,"engine":null,"newTab":false,"pinned":false,"isPrivate":false,"delayLoad":false,"url":"https://www.mozilla.com","parentId":-1,"selected":true,"desktopMode":false} Maybe the fact that you're navigating away from the test page (through `NavigationHelper.goBack`) is interfering with your event listener.
Flags: needinfo?(nchen)
Also, because of the way JavascriptBridge works, I don't think the `java.asyncCall` calls in JS are getting delivered to the Java side. You need a corresponding `getJS().syncCall` call on the Java side in order to receive incoming calls from JS. The reason is the test thread is normally running your test code, so there is no chance for incoming calls from JS to run on the test thread. `getJS().syncCall` is the only place where we check for incoming calls from JS and deliver those calls on the test thread. So in case you're relying on `java.asyncCall` to check if your event listener is getting called, the real issue may be that the `java.asyncCall` calls are not getting delivered.
tracking-fennec: ? → +
Priority: -- → P1
Iteration: --- → 1.29
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review181488 Some problems here due to how Try/builders use the keys, see comment. I also see some lint failures. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testActivityStreamPocketReferrer.java:70 (Diff revision 3) > + Log.d(LOGTAG, "testReferrerInTopStories"); > + > + WaitHelper.waitForPageLoad(new Runnable() { > + @Override > + public void run() { > + mSolo.clickOnText(PocketStoriesLoader.PLACEHOLDER_TITLE); // Click Top Story placeholder item. Okay so, there's some annoying problems here from the way I wrote the Pocket work to work on builders - I see a try failure, but that's because the try server doesn't have access to the keys, but it tries to build with dummy keys so it hits the Pocket servers but they don't return anything. Depending on what is built on the server (e.g. nightly), the build uses the real keys, so it will just load live Pocket items, and this test will fail. Is it possible to just "open the first Pocket story"? and then check that to see if the url includes the referer id?
Attachment #8902969 - Flags: review?(liuche)
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review181488 > Okay so, there's some annoying problems here from the way I wrote the Pocket work to work on builders - I see a try failure, but that's because the try server doesn't have access to the keys, but it tries to build with dummy keys so it hits the Pocket servers but they don't return anything. > > Depending on what is built on the server (e.g. nightly), the build uses the real keys, so it will just load live Pocket items, and this test will fail. > > Is it possible to just "open the first Pocket story"? and then check that to see if the url includes the referer id? > Depending on what is built on the server (e.g. nightly), the build uses the real keys, so it will just load live Pocket items, and this test will fail. Robocop tests will automatically fail if we try to make a network connection (at least through Gecko, which is why I had to change the URL endpoints when clicking pocket items) – I wonder why we aren't see more failures when Pocket initially tries to load? (Or are we seeing silent ones?) Or is the `ProxySelector` smart enough to force those connections to hang during tests? (the screenshot for my failed push showed 0 pocket items). That being said, my test also failed because the welcome dialog would push pocket items off the screen and there would be nothing to click. I'll rebase and see the try results.
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review181488 > > Depending on what is built on the server (e.g. nightly), the build uses the real keys, so it will just load live Pocket items, and this test will fail. > > Robocop tests will automatically fail if we try to make a network connection (at least through Gecko, which is why I had to change the URL endpoints when clicking pocket items) – I wonder why we aren't see more failures when Pocket initially tries to load? (Or are we seeing silent ones?) Or is the `ProxySelector` smart enough to force those connections to hang during tests? (the screenshot for my failed push showed 0 pocket items). > > That being said, my test also failed because the welcome dialog would push pocket items off the screen and there would be nothing to click. I'll rebase and see the try results. Again no pocket items ([screenshot](https://public-artifacts.taskcluster.net/Plqrd_-jQye9vP--UbeQ0Q/0/public/test_info//robocop-screenshot-org.mozilla.gecko.tests.testActivityStreamPocketReferrer.jpg)) – I wonder if the request is hanging.
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review181488 > Again no pocket items ([screenshot](https://public-artifacts.taskcluster.net/Plqrd_-jQye9vP--UbeQ0Q/0/public/test_info//robocop-screenshot-org.mozilla.gecko.tests.testActivityStreamPocketReferrer.jpg)) – I wonder if the request is hanging. I chose to force load the placeholders to solve this issue.
Iteration: 1.29 → 1.30
Not important: can do after merge.
Rank: 2
Priority: P1 → P2
Iteration: 1.30 → ---
All open Activity Stream bugs are moving from the whiteboard tag, "[mobileAS]", to the Firefox for Android component, "Activity Stream", so that I can keep better track of these bugs as the new triage owner; I will send out an email shortly with additional details, caveats, etc.
Component: Testing → Activity Stream
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review188574 Some confusion over how this test runs, but logic is sound. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testActivityStreamPocketReferrer.java:26 (Diff revision 5) > + * - Clicking or long clicking a Pocket top story has a Pocket referrer. > + * - Clicking or long clicking other items on top sites does not have a Pocket referrer. > + * > + * The ideal test is to set up a server that will assert that clicks that should have a Pocket referrer > + * have one in the request headers and clicks that should not have a referrer do not have one. However, > + * it's non-trivial to set up such a server in our harness so we instead intercept Tab:Load JS events Talked irl, but this was confusing because I thought this interception and checking would be done at the same time, rather than intercepting Tab:Load, setting a flag, and then checking it later (due to constraints on the JS bridge). I did read through some of the JSBridge code but ideally this comment could just explain everything and no one needs to go read JavaScriptBridge\*.\* ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testActivityStreamPocketReferrer.java:90 (Diff revision 5) > + public boolean isSatisfied() { > + return !mSolo.searchText(StringHelper.get().CONTEXT_MENU_OPEN_IN_NEW_TAB); > + } > + }, 5000); > + > + // We really need to block for Tab:Load but there's no simple way to wait for This comment was confusing because I was in the mindset of "listening and checking at the same time" so sleeping seems like you run the risk of missing the message! ::: mobile/android/tests/browser/robocop/testActivityStreamPocketReferrer.js:16 (Diff revision 5) > + > + java.disconnect(); > +}); > +do_test_pending(); > + > +var isTabLoadReceived = false; wasTabLoadReceived would make it clearer that this is a flag being set.
Attachment #8902969 - Flags: review?(liuche) → review+
Comment on attachment 8902969 [details] Bug 1395409: Add testActivityStreamPocketReferrer. https://reviewboard.mozilla.org/r/174742/#review188574 > Talked irl, but this was confusing because I thought this interception and checking would be done at the same time, rather than intercepting Tab:Load, setting a flag, and then checking it later (due to constraints on the JS bridge). I did read through some of the JSBridge code but ideally this comment could just explain everything and no one needs to go read JavaScriptBridge\*.\* I changed the method titles to things similar to: `assertTabLoadEventContainsPocketReferrer` and added a comment to that method: ```java // We intercept the Tab:Load event in JS and, due to limitations in JavascriptBridge, // store the data there until Java asks for it. ``` > This comment was confusing because I was in the mindset of "listening and checking at the same time" so sleeping seems like you run the risk of missing the message! Changed the comment to: ```java // There's no simple way to block until a background page loads so instead, we sleep for 500ms. // Our JS listener is attached the whole time so if the message is sent, we'll receive it and cache it // while we're sleeping. ```
Pushed by michael.l.comella@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ffd9345660e5 Add testActivityStreamPocketReferrer. r=liuche
https://hg.mozilla.org/mozilla-central/rev/ffd9345660e5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
This is a test to catch regressions so it isn't super important to uplift it.
status-firefox57: affected → wontfix
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: