1558486 - remove test_client_focus.html

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

test_client_focus.html is not referenced anywhere, so is never run:

https://searchfox.org/mozilla-central/search?q=test_client_focus.html&case=false&regexp=false&path=

In my work for bug 1558485 I detected there's a misstyped "return" statement which is how I found this issue.

As far as I can tell, the last changeset affecting this test was this one:

https://hg.mozilla.org/mozilla-central/rev/23e43cedb8e9

This removed test_client_focus.html from the manifest, but it also changed a comment in test_client_focus.html itself.

There's no comments in bug that reflect if it should have been removed or not.

Ehsan, looks like you and dao reviewed it, any ideas?

Comment 1

[(no longer active)]

The removal from the manifest looks accidental to me. That being said, the test is old, so it is now of questionable value. Andrew, what do you think?

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to :Ehsan Akhgari from comment #1)

The removal from the manifest looks accidental to me. That being said, the test is old, so it is now of questionable value. Andrew, what do you think?

Andrew is busy with reviewing.
Hey Perry, I wonder if you have thoughts here.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

The test looks out of date to me and can safely be removed.

• It's testing client.focus(), which now can only be performed in response to a user interaction event (e.g. notificationclick), which is what the test test_notificationclick_focus.html added in https://hg.mozilla.org/mozilla-central/rev/23e43cedb8e9 is doing.
• The last change to the test replaces the comment describing its purpose as "This test checks that client.focus is available.", which suggests that the changeset author had meant to change the code in the file too, but it seems to me that test_notificationclick_focus.html checks client.focus()'s existence by testing it itself.
• It's auxiliary worker script and iframe HTML files were removed too.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Perry!

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1558486 - remove abandoned, archaic test_client_focus.html test r=#dom-workers

Per Perry's analysis in
https://bugzilla.mozilla.org/show_bug.cgi?id=1558486#c3 and my
independent rediscovery of Bug 1144660 having disabled/broken it when
:julienw raised the issue of the test appearing broken, the test file
which is not covered by any mochitest.ini and is missing its support
files should be removed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b168fd22e8d
remove abandoned, archaic test_client_focus.html test r=dom-worker-reviewers,sg

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2b168fd22e8d