1333783 - Add tests to check that geolocation is still working

Status: VERIFIED WONTFIX

(Core :: DOM: Geolocation, defect, P1)

Description

[Sylvestre Ledru [:Sylvestre]]

We shipped a release (51.0) with the feature broken for more than a month (beta too).

We should have automation to check that it is working.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andrew, do you know who could write this test? Thanks

Comment 2

[Hanno Schlichting]

My guess is Michelangelo De Simone might be the right person - IIRC he's doing most of the geolocation work these days.

For testing there might be overlap here to also make sure safebrowsing is working, both of these features need the API keys in place and both might have similar test setup problems with talking to an external service.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

OK, thanks!

Comment 4

[Andrew Overholt [:overholt]]

We could also ask Kan-Ru.

Comment 5

[Michelangelo De Simone [Offline] [:mds]]

There is no single solution here.
Given the nature of the feature - and its current status - this is my take on what geo needs going on.

1. In geo we must hard assert for non-default and non-empty API keys wherever it makes sense. Even though it's not nearly as reliable as we need, it's easy and cheap.

2. We need to start checking and VERIFYING the *ACTUAL* API keys we use since Nightly. I'm not a fan of functional tests relying on third-party services like this but TBH I don't see any other way, also because this will increase our numbers with our network providers. Feel free to propose smarter solutions.

3. Native Providers

We need to go native, aggressively.
The current situation is a rats nest and too often we go asking the network, even when we have native providers.
This change is huge for a number of reasons: AFAICT the Win provider isn't tested and I'm not even sure it actually works as intended.

4. SecureContext by default

Not strictly related to today's events, but it needs to happen.

Comment 6

[Hanno Schlichting]

(In reply to Michelangelo De Simone [:mds] from comment #5)
> Given the nature of the feature - and its current status - this is my take
> on what geo needs going on.

+100 on improving the geo code and tests :)
 
> 1. In geo we must hard assert for non-default and non-empty API keys
> wherever it makes sense. Even though it's not nearly as reliable as we need,
> it's easy and cheap.

+1 the tricky bits were getting keys shipped to all try/build machines. While we still did Firefox OS, there were also quite a number of people who developed locally and relied on geolocation to be working for development of all kinds of apps. On desktop, the number of developers that actually build Firefox locally and need geolocation to work is much smaller, so this shouldn't be a problem anymore.

> 2. We need to start checking and VERIFYING the *ACTUAL* API keys we use
> since Nightly. I'm not a fan of functional tests relying on third-party
> services like this but TBH I don't see any other way, also because this will
> increase our numbers with our network providers. Feel free to propose
> smarter solutions.

+1 I don't like functional tests depending on external services either, but here that seems to be the only way to actual test the full functionality.

> 3. Native Providers
> 
> We need to go native, aggressively.
> The current situation is a rats nest and too often we go asking the network,
> even when we have native providers.
> This change is huge for a number of reasons: AFAICT the Win provider isn't
> tested and I'm not even sure it actually works as intended.

Native providers are tricky. Our Geolocation contract doesn't allow us to use any secondary Geolocation providers, be that other service or native in our release channel. So unless we renegotiate terms, we can't use any native providers in the release channel. We choose to also make beta the same as release, in order to test what we'd be shipping to release. For Aurora/Nightly we use our own geo service and the native Mac OS Core Location provider, though there's more or less only historical reasons for it.

The Mac OS Core Location provider has a fallback on our geo service, as Core Location doesn't give back any results in case you have no WiFi networks around you, so we use our service as a GeoIP fallback.

The Windows provider was a prototype that was enabled for a short while in one of the channels as a test. We looked at the quality and reach of the service and surprisingly the quality turned out to be worse than Mozilla's own geo service. Since the Windows provider requires at least Windows 8+ it was also not covering a lot of our user base at the time of that experiment, so overall it wasn't worthwhile.

Finally there's a whole bunch of GPS related functionality in geo, which I think could be removed, as we aren't doing Firefox OS anymore. On our actual mobile browsers we use none of this and stick with the Android/iOS operating system provided services.

If we wanted to simplify things, the easiest path forward would be to remove all the native providers and stick with the network location provider and the network cache it implements alone. That still allows an end user to switch the backend geo service via geo.wifi.uri and use our geo service or one of the others out there that support the same network protocol.

Maybe that kind of removal of all native providers would be a good first step, which would make the code a lot easier to understand, refactor the more minimal code base and than add back native providers in a way that makes sense. Right now the code is incredibly hard to understand and not written with any kind of testing in mind. But that's just my outside perspective ;)

> 4. SecureContext by default
> 
> Not strictly related to today's events, but it needs to happen.

+0.5 pending a clearer decline from our usage numbers

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Hanno Schlichting [:hannosch] from comment #6)
> (In reply to Michelangelo De Simone [:mds] from comment #5)
> > Given the nature of the feature - and its current status - this is my take
> > on what geo needs going on.
> 
> +100 on improving the geo code and tests :)

+1000 ;)
  
> > 1. In geo we must hard assert for non-default and non-empty API keys
> > wherever it makes sense. Even though it's not nearly as reliable as we need,
> > it's easy and cheap.

Agree

> > 2. We need to start checking and VERIFYING the *ACTUAL* API keys we use
> > since Nightly. I'm not a fan of functional tests relying on third-party
> > services like this but TBH I don't see any other way, also because this will
> > increase our numbers with our network providers. Feel free to propose
> > smarter solutions.

You mean only checking the API key used but not actually running tests with the API key right? I don't think it'll work on our test automation environment.

We can probably deploy a telemetry probe to monitor the timeouts or errors occurred when making network requests. Then if something went wrong we will be noticed.

> > 3. Native Providers
> > 
> > We need to go native, aggressively.
> > The current situation is a rats nest and too often we go asking the network,
> > even when we have native providers.
> > This change is huge for a number of reasons: AFAICT the Win provider isn't
> > tested and I'm not even sure it actually works as intended.
 
IMO we should be aiming for full native providers where possible and only use network providers as fullback. We can also split the pre-release channels to use different providers for testing purpose.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #7)
> IMO we should be aiming for full native providers where possible and only
> use network providers as fullback. We can also split the pre-release
> channels to use different providers for testing purpose.

I agree that using native providers would be good because we might get cached OS results for fast or offline lookups. However, splitting the pre-release channels to use different providers than the release channel means we get less testing of the code we actually ship and we risk missing bugs such as API key bug 1333516.

Comment 9

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Chris Peterson [:cpeterson] from comment #8)

> I agree that using native providers would be good because we might get
> cached OS results for fast or offline lookups. However, splitting the
> pre-release channels to use different providers than the release channel
> means we get less testing of the code we actually ship and we risk missing
> bugs such as API key bug 1333516.

This is not entirely the case.
The network provider (whatever that is) needs to be always ready to use, because it's either our first and only choice when we lack a native provider, or because we need it as a backup when the native provider gives us bananas.

Now, to avoid problems like the one that led to bug 1333516, I'd *love* to have one simple smoke test that forces the use of the REAL network provider. That would be, probably, the best and cleanest solution.

This hypothetical test should be run manually when RelEng is about to release a new Aurora/Beta/Release because the API key(s) should be part of the build in that moment.

I have no clue whether we have some manual QA pipeline in place for cases like this.
Ni'ing somebody that may know.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Michelangelo De Simone [:mds] from comment #9)
> This hypothetical test should be run manually when RelEng is about to
> release a new Aurora/Beta/Release because the API key(s) should be part of
> the build in that moment.

There was a separate email thread going with our QA people about this exact topic and I believe that was the same conclusion (adding some basic smoketesting to the release signoff process).

Comment 11

[Michelangelo De Simone [Offline] [:mds]]

Geolocation will be validated manually and the smoke tests will be part of the next signoffs.
The first affected signoff will be for 52 Beta.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

The issue wasn't fixed as we were talking about automated tests.
Smoke tests don't catch issues when they aren't on the product side.