Bug 27716 – Add Geolocation layout tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Geolocation was implemented in changeset http://trac.webkit.org/changeset/37854 (bug https://bugs.webkit.org/show_bug.cgi?id=21475). This initial changeset added a layout test to check that navigator.geolocation is not present, but this was later removed in changeset http://trac.webkit.org/changeset/41798.

Bug https://bugs.webkit.org/show_bug.cgi?id=21717 was created to track the need to test Geolocation when the platform does not provide an implementation. This is achieved using a mock Geolocation implementation, but is complicated as navigator.geolocation needs to be present for testing, but should not be present otherwise.

As an intermediate solution, I suggest that Geolocation is tested only on platforms where the platform provides an implementation and hence navigator.geolocation is present. On other platforms, we simply test that navigator.geolocation is absent.

What do people think?

[Steve Block]

Created attachment 33625 [details]
Patch for bug 27716

A patch to add the first couple of unit tests, which simply test that navigator.geolocation is present only when is is supposed to be.

[Eric Seidel (no email)]

I think these tests are OK, but not really necessary.  I do think we should provide some sort of mock-geolocation system via DumpRenderTree and a geolocationController.

[Steve Block]

> I do think we should
> provide some sort of mock-geolocation system via DumpRenderTree and a
> geolocationController.
Agreed. I'm currently working on exactly this and will send patches.

What I wanted to get agreement on now, is that on platforms where Geolocation is not implemented, we should not test it via a mock implementation, but simply test that navigator.geolocation is not present.

[Eric Seidel (no email)]

It's the individual platforms responsibility to manage their skipped lists and turn on/off tests accordingly.  Platforms which want to test for missing geolocation can simply turn on the geolocation tests and check in failing results. :)

I thikn it's also OK to have a "is geolocation enabled" test, expect enabled, and check in "PASS" results on the platforms which have it, and skip it on all other platforms.  But that test is also implied by your more complicated tests you're about to add/write.

[Steve Block]

> It's the individual platforms responsibility to manage their skipped lists and
> turn on/off tests accordingly.  Platforms which want to test for missing
> geolocation can simply turn on the geolocation tests and check in failing
> results. :)
OK, I'll dispense with the 'not-enabled' test then.

> I thikn it's also OK to have a "is geolocation enabled" test, expect enabled,
> and check in "PASS" results on the platforms which have it, and skip it on all
> other platforms.
I'll update the patch to add only the 'enabled' test and not add it to any skip lists.

>  But that test is also implied by your more complicated tests
> you're about to add/write.
Sure, but I think there's value in testing that the geolocation object has the correct type and that the navigator object's properties enumerate correctly.

[Eric Seidel (no email)]

(In reply to comment #5)
> >  But that test is also implied by your more complicated tests
> > you're about to add/write.
> Sure, but I think there's value in testing that the geolocation object has the
> correct type and that the navigator object's properties enumerate correctly.

I certainly agree!  And I wish we had more tests of our DOM bindings.

[Steve Block]

Created attachment 33716 [details]
Patch for bug 27716

[Steve Block]

Comment on attachment 33716 [details]
Patch for bug 27716

It seems I forgot to set the review flag for this patch when I uploaded it.

[Steve Block]

Adding Greg to CC.

[Darin Adler]

Comment on attachment 33716 [details]
Patch for bug 27716

Please put these tests into fast/dom/Geolocation instead of a new top-level directory.

[Steve Block]

Created attachment 34280 [details]
Patch 3 for bug 27716

Moved test to fast/dom/Geolocation as requested.

[Greg Bolsinga]

I'm not a reviewer, but looks good to me.

[Eric Seidel (no email)]

Comment on attachment 34280 [details]
Patch 3 for bug 27716

This really should be a one of the new-style JS tests.  see fast/js/resources and make-js-test-wrappers.

That would give you shouldBe(), and debug(), etc. for free.

It looks like someone has duplicated at least part of that functionality with the "utils.js" file.

[Eric Seidel (no email)]

I think this patch is OK as is, but I'd really rather see it using the same framework as most of the other new tests use.

[Steve Block]

Created attachment 34402 [details]
Patch 4 for bug 27716

Now using JS test framework from fast/js/resources.

[Eric Seidel (no email)]

Comment on attachment 34402 [details]
Patch 4 for bug 27716

Hot hot hot! :)

hasGeolocationProperty() is really testing "isGeolocationPropertyEnumerable" :)

 13 shouldBeTrue("typeof navigator.geolocation == 'object'");

could be written as:
shouldBeEqualToString("typeof navigator.geolocation", "object");

But it looks great as is too! :)

Thanks!

[Eric Seidel (no email)]

Comment on attachment 34402 [details]
Patch 4 for bug 27716

This is missing new results and cannot be landed by the commit-queue.

[Steve Block]

> This is missing new results and cannot be landed by the commit-queue.
The patch includes expected results for GTK at LayoutTests/platform/gtk/fast/dom/Geolocation/enabled-expected.txt.

Perhaps I misinterpreted your earlier comment ...
> It's the individual platforms responsibility to manage their skipped lists and
> turn on/off tests accordingly.
>
> I thikn it's also OK to have a "is geolocation enabled" test, expect enabled,
> and check in "PASS" results on the platforms which have it, and skip it on all
> other platforms.

Did you mean that it's the platform's responsibility for all platforms other than mac? Should I add a 'fail' result for mac or add this test to the mac skipped list?

[Eric Seidel (no email)]

The commit-queue runs from a mac.  Should these results be the same for all platforms?  If so, then they should be placed right next to the original test and platforms which want to check in platform-specific failing results can do so.

[Steve Block]

> The commit-queue runs from a mac.  Should these results be the same for all
> platforms?
The 'pass' results should be the same for all platforms that support Geolocation. Currently, this is only GTK. On platforms that don't support Geolocation (which includes mac), all tests will fail.

Should I add 'failed' results to LayoutTests/fast/dom/ and 'pass' results to LayoutTests/platform/gtk/fast/dom/Geolocation/? Or should I add the 'pass' results to LayoutTests/fast/dom/ and add 'failed' results to LayoutTests/platform/mac/fast/dom/Geolocation/ to keep the build happy?

[Eric Seidel (no email)]

The failure case seems uninteresting.  I would checkin the results next to the tests, and edit the skipped lists of all platforms you expect to fail.

[Steve Block]

Created attachment 34671 [details]
Patch 5 for bug 27716

> The failure case seems uninteresting.  I would checkin the results next to the
> tests, and edit the skipped lists of all platforms you expect to fail.
Done. I was confused by your earlier comments about maintenance of the skipped lists being the responsibility of the individual platforms.

[Eric Seidel (no email)]

Comment on attachment 34671 [details]
Patch 5 for bug 27716

LGTM.

[Eric Seidel (no email)]

Comment on attachment 34671 [details]
Patch 5 for bug 27716

Rejecting patch 34671 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34671 from bug 27716 failed to download and apply.

[Eric Seidel (no email)]

Applying 34671 from bug 27716.
[snip]
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 89.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej
patch -p0 "LayoutTests/platform/mac/Skipped" returned 1.  Pass --force to ignore patch failures.
Rejecting patch 34671 from commit-queue.  This patch will require manual commit.

mac/Skipped has changed.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/Geolocation/enabled-expected.txt
	A	LayoutTests/fast/dom/Geolocation/enabled.html
	A	LayoutTests/fast/dom/Geolocation/resources/TEMPLATE.html
	A	LayoutTests/fast/dom/Geolocation/resources/enabled.js
	M	LayoutTests/platform/mac/Skipped
Committed r47168
	M	LayoutTests/platform/mac/Skipped
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/Geolocation/enabled-expected.txt
	A	LayoutTests/fast/dom/Geolocation/resources/enabled.js
	A	LayoutTests/fast/dom/Geolocation/resources/TEMPLATE.html
	A	LayoutTests/fast/dom/Geolocation/enabled.html
r47168 = 4c2f9fcb3b4be28142507c551e59d825778c701a (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47168

[Mark Rowe (bdash)]

<http://trac.webkit.org/changeset/47168> doesn't include the changes to qt/Skipped or win/Skipped.  Presumably this should also be skipped for GTK too.

[Mark Rowe (bdash)]

Comment on attachment 34671 [details]
Patch 5 for bug 27716

> +    for (var property in navigator) {
> +        if (property == "geolocation") {
> +            return true;
> +        }
> +    }

We don't include braces around the body of single-line if statements.

[Eric Seidel (no email)]

The patch has the changes, so I must have just applied it wrong somehow.

[Steve Block]

> <http://trac.webkit.org/changeset/47168> doesn't include the changes to
> qt/Skipped or win/Skipped.
> We don't include braces around the body of single-line if statements.
Both of these are fixed in http://trac.webkit.org/changeset/47169. Thanks Eric!

[Steve Block]

Created attachment 34725 [details]
Disables Geolocation tests for GTK

Disables Geolocation tests for GTK, at the request of [e-mail].

[David Levin]

Comment on attachment 34725 [details]
Disables Geolocation tests for GTK

> Index: LayoutTests/platform/gtk/Skipped
> @@ -5979,3 +5979,6 @@ editing/selection/find-in-text-control.h
>  fast/harness/override-preferences.html
>  fast/harness/override-preferences-2.html
>  fast/harness/override-zzz-reset.html
> +
> +# This port doesn't support Geolocation.
> +fast/dom/Geolocation

Two issues:
* According to comment at the top of the file: "no grouping should occur. Every test for itself."

* This appears to be doing the addition in the section which is "# Tests with bugs attached : Enable these tests again once these bugs are fixed." but this failure doesn't fit in that category.

[Steve Block]

Created attachment 34727 [details]
Disables Geolocation tests for GTK, Patch 2

Sorry, didn't see those rules.

[Steve Block]

Created attachment 34740 [details]
Adds a mock Geolocation service, Patch 1

Adds a mock Geolocation service for use in LayoutTests.

[Steve Block]

Created attachment 34741 [details]
Adds a mock Geolocation service, Patch 2

Patch now includes additions to makefiles, xcode project etc.

[Eric Seidel (no email)]

In general we should use new bugs for new patches.  Twill be less confusing to reviewers and our tools. :)

[Eric Seidel (no email)]

Comment on attachment 34727 [details]
Disables Geolocation tests for GTK, Patch 2

Clearing flags on attachment: 34727

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/gtk/Skipped
Committed r47204
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
r47204 = 634ae99c08d1c4c5665a1e7608861c350d25570c (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47204

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Case in point.  We were just bitten by bug 28230.

[Steve Block]

I've moved the patch to add the mock Geolocation service to a new bug - Bug 28264.