1313096 - Add testing for Clients.openWindow() on Fennec

Status: NEW

(Core :: DOM: Service Workers, defect, P3)

Description

[Dylan Roeh (:droeh) (he/him)]

We need to add tests for Clients.openWindow on Fennec to ensure it works in various cases (eg when Fennec is backgrounded or killed).

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I am refactoring openWindow() in bug 1293277.  Not having a test for fennec really makes this painful.  It would be very helpful if someone familiar with fennec and its test infrastructure could get this test stood up.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Dylan can you work on some sort of basic test for this?

Comment 3

[Dylan Roeh (:droeh) (he/him)]

What exactly are we looking for in a test here? Simply testing openWindow() while Fennec is running and foregrounded, or testing it while Fennec is backgrounded or even killed as I mentioned in the description? I think the former is doable, but if I recall correctly the latter requires interacting with a notification; when we talked about it around the time I filed this bug, that seemed like something we weren't really able to do.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right, the former.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Right, the former.

I don't think we can just ignore the background or killed cases.  For example, in my day-to-day use of twitter mobile and telegram web on fennec I often see clicking a notification do nothing.  It seems these cases have regressed and we didn't notice.

Can we simulate a notificationclick?  I think we have the ability to simulate a push event.

At this point, though, I'll take whatever test I can get to help verify I'm not causing further regressions with my rewrite.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #5)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > Right, the former.
> 
> I don't think we can just ignore the background or killed cases.  For
> example, in my day-to-day use of twitter mobile and telegram web on fennec I
> often see clicking a notification do nothing.  It seems these cases have
> regressed and we didn't notice.
> 
> Can we simulate a notificationclick?  I think we have the ability to
> simulate a push event.
> 
> At this point, though, I'll take whatever test I can get to help verify I'm
> not causing further regressions with my rewrite.

We cannot simulate an actual notification click. We could mock the notification interface so that it doesn't actually display one and then "click" that, though. Still, it's going to be very difficult to test the background case with our current harnesses. Creating a new one for this type of usage would be a lot of work IMHO. Our best play may be to make sure it gets covered in the manual test plans.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Also, I know this is not what people want to hear, but if you're doing major refactoring and have concerns about regressing a particular platform, you should do local development/testing on that platform.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

> We cannot simulate an actual notification click. We could mock the
> notification interface so that it doesn't actually display one and then
> "click" that, though. Still, it's going to be very difficult to test the
> background case with our current harnesses. Creating a new one for this type
> of usage would be a lot of work IMHO. Our best play may be to make sure it
> gets covered in the manual test plans.

A simulated notification+click like that would be an improvement over the current situation and would fully test the openWindow paths.  Can we at least test the background-but-not-killed case with our current test infrastructure?

Where are the manual test plans?  Do they have anything for background push notification and opening a window?  If so, that would give me more confidence in changing this stuff.

> Also, I know this is not what people want to hear, but if you're doing major
> refactoring and have concerns about regressing a particular platform, you
> should do local development/testing on that platform.

I'm planning to, but I have low confidence in the code I have to start from because AFAIK it has not be tested regularly.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

So, we do have this mochitest that runs on fennec today:

  dom/workers/test/serviceworkers/test_openWindow.html

It only handles foreground use cases, though.

It sounds like the thing that missing is the background and not-running use cases.

Comment 10

[SUN Haitao]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #5)
> Can we simulate a notificationclick?  I think we have the ability to
> simulate a push event.

I believe all you need is using an intent like this:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java#99

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

It sounds like this is blocked on automation issues.  I plan to move forward on bug 1293277 without it.