1405477 - Pocket needs some automated tests

Status: RESOLVED FIXED

(Firefox :: Pocket, enhancement, P3)

Description

[:Gijs (he/him)]

As it is, the only test we have only checks that its UI is somewhere in the DOM, not that it actually works. So we break it (bug 1389721) and it seems the bugzilla component isn't strongly owned so then the breakage goes unnoticed. :-(

We should:
- add a pref for the target of the API calls if there isn't already
- add a test sjs thing that we use instead, when running mochitest browsers in this directory (can set the pref in the manifest)
- write tests for:
-- the page action item saving to pocket
-- the page action urlbar item saving to pocket
-- the various "open pocket list" menus opening the right tabs
-- the context menu working
and any other UI entry points I've forgotten about.

I'm happy to help mentor people here, but I don't have cycles to do this myself in the short term.

Comment 1

[:Gijs (he/him)]

Would love to mentor, but not a 57 priority.

Comment 2

[:Gijs (he/him)]

Realistically I don't think people are interested in taking this as a mentored bug.

Comment 3

[Sean Reeise [:sreeise]]

(In reply to :Gijs (out Thu 27 - Sun 30 / 9;  he/him) from comment #2)

I would more then happy to help with this and work on writing tests as well as mentored if you have time, but I can just as much learn as I go too. I've worked on some tests and know the basics of mochitest and xpcshell unit tests, so I understand to a certain extent how to write them.  

Have any of these tests been done yet? Or do bugs need to be created for each one? If you have any more information about what you are looking for in the tests as well, that would be great.

Comment 4

[:Gijs (he/him)]

(In reply to Sean Reeise [:sreeise] from comment #3)
> (In reply to :Gijs (out Thu 27 - Sun 30 / 9;  he/him) from comment #2)
> 
> I would more then happy to help with this and work on writing tests as well
> as mentored if you have time, but I can just as much learn as I go too. I've
> worked on some tests and know the basics of mochitest and xpcshell unit
> tests, so I understand to a certain extent how to write them.  
> 
> Have any of these tests been done yet?

Not really. There's a test that checks that the UI elements get created ( https://searchfox.org/mozilla-central/source/browser/extensions/pocket/test/browser_pocket_ui_check.js ) but nothing else.

> Or do bugs need to be created for each one?

You could create some dependent bugs for individual tests, or write several in here.

> If you have any more information about what you are looking for in
> the tests as well, that would be great.

Basically, this list in comment #0:

(In reply to :Gijs (he/him) from comment #0)
> - write tests for:
> -- the page action item saving to pocket
> -- the page action urlbar item saving to pocket
> -- the various "open pocket list" menus opening the right tabs
> -- the context menu working

These would need to be browser mochitests. To test these things, we'll need to make sure we can force the pocket add-on to contact example.com/example.org instead of getpocket.com or something (as you might have already found, we can't contact remote servers inside automated tests) using a preference which we set to a test URL. We probably want to check that the right panel opens when trying to save to pocket, and that that panel opens the right URL (in this case, the substituted example.com/example.org one).

Does that help? It's cool you're interested in working on this - it would really help our test coverage if we had some tests covering basic functionality here.

Comment 5

[Sean Reeise [:sreeise]]

> These would need to be browser mochitests. To test these things, we'll need
> to make sure we can force the pocket add-on to contact
> example.com/example.org instead of getpocket.com or something (as you might
> have already found, we can't contact remote servers inside automated tests)
> using a preference which we set to a test URL. We probably want to check
> that the right panel opens when trying to save to pocket, and that that
> panel opens the right URL (in this case, the substituted
> example.com/example.org one).
> 
> Does that help? It's cool you're interested in working on this - it would
> really help our test coverage if we had some tests covering basic
> functionality here.

Thanks, yeah it does. I'll go through the pocket code to see how it works to help better understand how to write the tests. I did want to ask about one part, I understand what you mean by forcing pocket to go to a different url, I guess I am just wondering if there is a specific way we would need to do this. I know we set the urls in Pocket.jsm and pktApijsm, so I assume the tests would somehow need to change the url first and then trigger the the page action?

Comment 6

[:Gijs (he/him)]

(In reply to Sean Reeise [:sreeise] from comment #5)
> I understand what you mean by forcing pocket to go to a different url,
> I guess I am just wondering if there is a specific way we would need to do
> this. I know we set the urls in Pocket.jsm and pktApijsm, so I assume the
> tests would somehow need to change the url first and then trigger the the
> page action?

Yes. Normally speaking the pocket jsms should be watching the pref for changes (or just fetching it every time it's needed) and so if the test sets it that's enough. Generally, inside an async test function (ie passed to add_task()), you'd do something like:

await SpecialPowers.pushPrefEnv({set: [["extensions.pocket.site", "example.com/path/to/some/test/file"]]});

which would set the pref.

Comment 7

[:Gijs (he/him)]

The pushPrefEnv notation (rather than just directly accessing the pref service) takes care of resetting any changed prefs at the end of the test (also if the test fails and stops halfway through), so different tests don't interfere with each other.

Comment 8

[Sean Reeise [:sreeise]]

Sorry I am just now getting back to this. I am working on the first test and started with the context menu save to pocket link. When a user is not logged in this should open the pocket menu sign up which is hanging down from the pocket button on the URL bar. I can test for this opening and that it is showing and everything works. The issue is that it still calls the show sign up command, triggering pocket's main.js and causing an error in the tests. 

Specifically I believe the command is:

oncommand="Pocket.savePage(gContextMenu.browser, gContextMenu.browser.currentURI.spec, gContextMenu.browser.contentTitle);"

from the context-pocket button which is the context menu button being tested. Is there a way to disable this, or simulate a click that doesn't cause the command to run? Thanks for your help.

Comment 9

[Sean Reeise [:sreeise]]

To add, the error is:

JavaScript error: chrome://pocket/content/main.js, line 166: ReferenceError: pktApi is not defined

which is from getFirefoxAccountSignedInUser(function(userdata) in main.js.

Comment 10

[:Gijs (he/him)]

(In reply to Sean Reeise [:sreeise] from comment #8)
> Sorry I am just now getting back to this. I am working on the first test and
> started with the context menu save to pocket link. When a user is not logged
> in this should open the pocket menu sign up which is hanging down from the
> pocket button on the URL bar. I can test for this opening and that it is
> showing and everything works. The issue is that it still calls the show sign
> up command, triggering pocket's main.js and causing an error in the tests. 
> 
> Specifically I believe the command is:
> 
> oncommand="Pocket.savePage(gContextMenu.browser,
> gContextMenu.browser.currentURI.spec, gContextMenu.browser.contentTitle);"
> 
> from the context-pocket button which is the context menu button being
> tested. Is there a way to disable this, or simulate a click that doesn't
> cause the command to run? Thanks for your help.

I'm a little confused. As part of the test, you click the menuitem and you check that the panel appears, right? My understanding is that if you don't run the command you referenced, the panel also won't appear...

(In reply to Sean Reeise [:sreeise] from comment #9)
> To add, the error is:
> 
> JavaScript error: chrome://pocket/content/main.js, line 166: ReferenceError:
> pktApi is not defined
> 
> which is from getFirefoxAccountSignedInUser(function(userdata) in main.js.

Hm. So this shouldn't really be happening - it sounds like a bug in pocket. But I don't see this error locally when using the context menu myself in a more-or-less clean profile (that's not signed in to pocket).

Can you post (as a gist or just as a phabricator or normal attachment review request for me) your work-in-progress test? That'll probably help me understand what's going wrong where/why better. Sorry for not having an answer immediately!

Comment 11

[Sean Reeise [:sreeise]]

Attachment: Bug 1405477 - Added automated tests for pocket r?Gijs

Added test for context menu button.

Comment 12

[Sean Reeise [:sreeise]]

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63dff1aef24f77f20654a48112eb989cf645543

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9df487a5fdfb
Added automated tests for pocket r=Gijs

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/9df487a5fdfb