w3c / geolocation

W3C Geolocation API
https://www.w3.org/TR/geolocation/
81 stars 56 forks source link

Investigate test failures to exit CR #120

Closed marcoscaceres closed 2 years ago

marcoscaceres commented 2 years ago

We have some failing tests on WPT which need to be investigated: https://wpt.fyi/results/geolocation-API?label=experimental&label=master&aligned

marcoscaceres commented 2 years ago

@reillyeon, @rakuco, sorry to bother you both, but it appears that on the Chromium side, the promise returned from test_driver.set_permission() never resolves (this is a regression, as I'm pretty sure it used to work). For example:

await test_driver.set_permission({ name: "geolocation" }, "granted");

That is what's causing most of the test failures in Chrome in the in the implementation report. Could either of your check if it's a known issues? Any suggestions about who we should contact?

The failures in other browsers are due to them not supporting test_driver.set_permission(), but the tests would otherwise all pass.

marcoscaceres commented 2 years ago

Actually, sorry, that's wrong... it does resolve, but it doesn't actually set the permission to "granted". Which then causes the tests to halt!

marcoscaceres commented 2 years ago

Ok, I've finished the investigation... test failures are to do with the test relying on test_driver.set_permission();.

However, from checking things manually, I think we are in a good state to proceed to Proposed Recommendation.

rakuco commented 2 years ago

Looking at https://wpt.fyi/results/geolocation-API?label=experimental&label=master&aligned, I see 3 tests failing:

rakuco commented 2 years ago

Erm, looking at the tests they don't seem to be calling test_driver.set_permission() at all... If I do that locally I can get getCurrentPosition_permission_deny.https.html to pass, but the allow version remains stuck waiting for a position (like in non-fully-active.https.html's case).

rakuco commented 2 years ago

Sigh, I should pay more attention to wpt.fyi before posting these comments...

marcoscaceres commented 2 years ago

Heh, I also should have commented that I’ve rewritten almost all the tests: https://github.com/web-platform-tests/wpt/labels/geolocation-API

(coincidentally, they need a review) but we are on the same track!

reillyeon commented 2 years ago

Can you figure out whether it is a WPT, WebDriver or Chrome issue that test_driver.set_permission() is not working correctly with Chrome when these tests are run through WPT? These tests are passing within the Chromium CI which means this is probably an issue somewhere in the WPT infrastructure or the libraries it depends on.

marcoscaceres commented 2 years ago

@reillyeon ok, that's good to hear... maybe it's just an issue on local machine?

Locally (MacOS), I do, for example:

./wpt run --binary=/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome chrome geolocation-API/getCurrentPosition_permission_allow.https.html

Or, Canary:

./wpt run --binary=/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary chrome geolocation-API/clearWatch_TypeError.https.html

In Canary, trying to get permission never resolves the promise.

In stable, the Geolocation prompt is unexpectedly shown (but at least I can click it to check the tests are ok).

Anyway, let's check wpt.fyi once the updated tests are merged... if it's running ok there, then everything is good.

marcoscaceres commented 2 years ago

Actually, ignore the above... I just realized I can do:

./wpt run --headless chrome path/to/whatever
marcoscaceres commented 2 years ago

Oh lol! Figured it out. The WPT-installed Chromium didn't have OS level permission to use Geolocation.

All good.