Update geofencing layout tests to use mock service.

Update geofencing layout tests to use mock service.

Depends on chromium side: https://codereview.chromium.org/701953007/

BUG=383125
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186927

[scheib, 2014-12-03 17:49:41]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2014-12-03 17:49:41]

I also believe we should include formal specification of the testing interface
necessary for a web spec. I've discussed this with a few Chromium folk, and also
James Graham <james@hoppipolla.co.uk> who's active in W3C testing at cwilso@'s
suggestion. I'll follow up more publicly on this after I have proven out the
model a bit.

Yes, I initially used internals as well and was informed this wasn't desirable
a) as internals is more for internals of Blink core, not the platform, and b)
larger functional tests are preferred, so having the control exist in content is
better. Bluetooth will be configuring the mocks from src/device/bluetooth and
configuring them in as a mock bluetooth adapter in content/browser. The layout
tests will be exercising the production code fully between content/browser to
blink js.

https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

For bluetooth I also am trying to minimize the testing interface. The plan is to
set a mock data set that will have known characteristics. The interface is then
just a string name for the data set. I will try to have events be
deterministically defined by the data set, e.g. configure "test suite 1" and get
a certain type of device that when you register for notifications on a given
characteristic it will automatically generate 2 of them, etc.

That approach perhaps makes more sense for bluetooth which has a sprawling set
of things (devices, services, characteristics, descriptors, events on those).
Geofencing does seem simpler, so I agree with the approach of enabling the mock
and then just setGeofencingPosition.

So, overall looks good but move the injection of testing code to content.

[Marijn Kruisselbrink, 2014-12-04 13:51:12]

On 2014/12/03 17:49:41, scheib wrote:
> I also believe we should include formal specification of the testing interface
> necessary for a web spec. I've discussed this with a few Chromium folk, and
also
> James Graham <mailto:james@hoppipolla.co.uk> who's active in W3C testing at
cwilso@'s
> suggestion. I'll follow up more publicly on this after I have proven out the
> model a bit.
> 
> Yes, I initially used internals as well and was informed this wasn't desirable
> a) as internals is more for internals of Blink core, not the platform, and b)
> larger functional tests are preferred, so having the control exist in content
is
> better. Bluetooth will be configuring the mocks from src/device/bluetooth and
> configuring them in as a mock bluetooth adapter in content/browser. The layout
> tests will be exercising the production code fully between content/browser to
> blink js.
> 
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
> 
> For bluetooth I also am trying to minimize the testing interface. The plan is
to
> set a mock data set that will have known characteristics. The interface is
then
> just a string name for the data set. I will try to have events be
> deterministically defined by the data set, e.g. configure "test suite 1" and
get
> a certain type of device that when you register for notifications on a given
> characteristic it will automatically generate 2 of them, etc.
> 
> That approach perhaps makes more sense for bluetooth which has a sprawling set
> of things (devices, services, characteristics, descriptors, events on those).
> Geofencing does seem simpler, so I agree with the approach of enabling the
mock
> and then just setGeofencingPosition.
> 
> So, overall looks good but move the injection of testing code to content.
I'm not sure what you mean with "move the injection of testing code to content"?
Somehow the geofencing layout tests will need to have a way to set the mock
geofencing position. As I said in the other CL, I'm not sure if this should be
per service worker registration, per origin, per storage partition, or chrome
global, but somehow this needs to be an API available to be called from the
layout test.
Long term this API would ideally be standardized, and be something like
"ServiceWorkerRegistration.geofencing.setMockPosition()" or maybe
"navigator.geolocation.setMockPosiiton", but until we reach that state some
layout tests only API, like something like this that is exposed via Internals
seems like the best thing to do?

Also having half of the test cases live in layouttests and the other half of the
same tests live in content also seems like it would make actually writing tests
very hard. And understanding what a test is doing even harder. You'd somehow
have to know about all these magic side effects of registering for notifications
etc.

[scheib, 2014-12-05 05:52:16]

On 2014/12/04 13:51:12, Marijn Kruisselbrink wrote:
> On 2014/12/03 17:49:41, scheib wrote:
> > I also believe we should include formal specification of the testing
interface
> > necessary for a web spec. I've discussed this with a few Chromium folk, and
> also
> > James Graham <mailto:james@hoppipolla.co.uk> who's active in W3C testing at
> cwilso@'s
> > suggestion. I'll follow up more publicly on this after I have proven out the
> > model a bit.
> > 
> > Yes, I initially used internals as well and was informed this wasn't
desirable
> > a) as internals is more for internals of Blink core, not the platform, and
b)
> > larger functional tests are preferred, so having the control exist in
content
> is
> > better. Bluetooth will be configuring the mocks from src/device/bluetooth
and
> > configuring them in as a mock bluetooth adapter in content/browser. The
layout
> > tests will be exercising the production code fully between content/browser
to
> > blink js.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
> > 
> > For bluetooth I also am trying to minimize the testing interface. The plan
is
> to
> > set a mock data set that will have known characteristics. The interface is
> then
> > just a string name for the data set. I will try to have events be
> > deterministically defined by the data set, e.g. configure "test suite 1" and
> get
> > a certain type of device that when you register for notifications on a given
> > characteristic it will automatically generate 2 of them, etc.
> > 
> > That approach perhaps makes more sense for bluetooth which has a sprawling
set
> > of things (devices, services, characteristics, descriptors, events on
those).
> > Geofencing does seem simpler, so I agree with the approach of enabling the
> mock
> > and then just setGeofencingPosition.
> > 
> > So, overall looks good but move the injection of testing code to content.
> I'm not sure what you mean with "move the injection of testing code to
content"?
> Somehow the geofencing layout tests will need to have a way to set the mock
> geofencing position. As I said in the other CL, I'm not sure if this should be
> per service worker registration, per origin, per storage partition, or chrome
> global, but somehow this needs to be an API available to be called from the
> layout test.
> Long term this API would ideally be standardized, and be something like
> "ServiceWorkerRegistration.geofencing.setMockPosition()" or maybe
> "navigator.geolocation.setMockPosiiton", but until we reach that state some
> layout tests only API, like something like this that is exposed via Internals
> seems like the best thing to do?
> 
> Also having half of the test cases live in layouttests and the other half of
the
> same tests live in content also seems like it would make actually writing
tests
> very hard. And understanding what a test is doing even harder. You'd somehow
> have to know about all these magic side effects of registering for
notifications
> etc.

(Repeating what we discussed in chat:)
Platform internals should be injected via testRunner, see review comments and
final patch here for the same issue I recently went through with bluetooth:
 https://codereview.chromium.org/650613005/

[Marijn Kruisselbrink, 2014-12-08 00:17:11]

On 2014/12/05 05:52:16, scheib wrote:
> (Repeating what we discussed in chat:)
> Platform internals should be injected via testRunner, see review comments and
> final patch here for the same issue I recently went through with bluetooth:
>  https://codereview.chromium.org/650613005/
Updates this CL to now only contain changes to the tests to use the testRunner
exposed geofencing mock.

[scheib, 2014-12-08 16:40:13]

lgtm with some optional suggestions.

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/event_triggering.html (right):

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/event_triggering.html:10: 
Maybe include a first test before anything that uses testRunner so that a casual
open of the test clearly explains why it's not running without needed to open
dev tools:

test(function(t) { assert_exists(window, "testRunner"); t.done(); },
     "window.testRunner is required for the following tests.");

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/event_triggering.html:44:
testRunner.setGeofencingMockPosition(37.422, -122.084015);
What is the initial position of the Mock? Should the test just set an initial
position?

[Marijn Kruisselbrink, 2014-12-11 04:02:16]

Patchset #6 (id:100001) has been deleted

[Marijn Kruisselbrink, 2014-12-11 04:43:37]

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/event_triggering.html (right):

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/event_triggering.html:10: 
On 2014/12/08 16:40:13, scheib wrote:
> Maybe include a first test before anything that uses testRunner so that a
casual
> open of the test clearly explains why it's not running without needed to open
> dev tools:
> 
> test(function(t) { assert_exists(window, "testRunner"); t.done(); },
>      "window.testRunner is required for the following tests.");

Done.

https://codereview.chromium.org/702983002/diff/80001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/event_triggering.html:44:
testRunner.setGeofencingMockPosition(37.422, -122.084015);
On 2014/12/08 16:40:13, scheib wrote:
> What is the initial position of the Mock? Should the test just set an initial
> position?
The initial position is "no position", which isn't a state that can otherwise be
reached currently, but it should be perfectly safe for the test not to set a
position before adding the first geofence.

[Marijn Kruisselbrink, 2014-12-11 04:43:42]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-11 04:44:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702983002/140001

[commit-bot: I haz the power, 2014-12-11 08:25:21]

Committed patchset #7 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186927