Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
172393006:
LayoutTest emulating geolocation with inspector (Closed)
|
Created:
6 years, 10 months ago by maheshkk Modified:
6 years, 10 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionLayoutTest emulating geolocation with inspector
Adding LayoutTests for emulating geolocation positions using inspector.
BUG=345040
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167810
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update review comments #
Total comments: 2
Patch Set 3 : Fixed as per review comments #
Total comments: 1
Patch Set 4 : removed sniffer code #
Total comments: 1
Patch Set 5 : Add sniffer code back #
Messages
Total messages: 15 (0 generated)
Please review.
https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... File LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt (right): https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt:8: error: Protocol Error: Invalid type of argument 'latitude' for method 'Geolocation.setGeolocationOverride' call. It must be 'number' but it is 'boolean'. This test only covers input parameters validation. Validation code is generated, so there is no need to test it for every command separately. https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... File LayoutTests/inspector/geolocation-emulation/emulate-geolocation-unavailable.html (right): https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... LayoutTests/inspector/geolocation-emulation/emulate-geolocation-unavailable.html:38: InspectorTest.runTestSuite([ These tests look great. But I'd rather place them all into a single TestSuite (i.e. testGeolocationUnavailable, testOverridenGeolocation, testNonOverridenGeolocation).
On 2014/02/20 11:38:42, pfeldman wrote: > https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... > File > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt > (right): > > https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt:8: > error: Protocol Error: Invalid type of argument 'latitude' for method > 'Geolocation.setGeolocationOverride' call. It must be 'number' but it is > 'boolean'. > This test only covers input parameters validation. Validation code is generated, > so there is no need to test it for every command separately. Agreed. I have still kept the unit test case but checking only for one use case. > > https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... > File > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-unavailable.html > (right): > > https://codereview.chromium.org/172393006/diff/1/LayoutTests/inspector/geoloc... > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-unavailable.html:38: > InspectorTest.runTestSuite([ > These tests look great. But I'd rather place them all into a single TestSuite > (i.e. testGeolocationUnavailable, testOverridenGeolocation, > testNonOverridenGeolocation). I rewrote all cases in one test suite now, this makes code lot cleaner and less code to maintain. Only downside I see is expected result text file is less readable than before when debug for failures IMO. Please take a look.
https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... File LayoutTests/inspector/geolocation-emulation-tests.html (right): https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... LayoutTests/inspector/geolocation-emulation-tests.html:69: GeolocationAgent.setGeolocationOverride(true, 500, 100); You are not calling next in the end of this section. It triggers because you have a console sniffer above. https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... File LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt (left): https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt:1: CONSOLE MESSAGE: line 11: Latitude: 100 Longitude: 200 These should not be a part of this patch - they don't exist on trunk.
On 2014/02/21 08:59:05, pfeldman wrote: Thanks for the review! > https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... > File LayoutTests/inspector/geolocation-emulation-tests.html (right): > > https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... > LayoutTests/inspector/geolocation-emulation-tests.html:69: > GeolocationAgent.setGeolocationOverride(true, 500, 100); > You are not calling next in the end of this section. It triggers because you > have a console sniffer above. > Ok, I will add next(). I was impression that, since this is last test case in the test suite, calling next would have no effect. > https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... > File > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt > (left): > > https://codereview.chromium.org/172393006/diff/70001/LayoutTests/inspector/ge... > LayoutTests/inspector/geolocation-emulation/emulate-geolocation-invalid-params-expected.txt:1: > CONSOLE MESSAGE: line 11: Latitude: 100 Longitude: 200 > These should not be a part of this patch - they don't exist on trunk. Sure, I will upload patch only with new files. I got confused whether I should undo the prev patch I uploaded for review before adding new files.
https://codereview.chromium.org/172393006/diff/130001/LayoutTests/inspector/g... File LayoutTests/inspector/geolocation-emulation-tests.html (right): https://codereview.chromium.org/172393006/diff/130001/LayoutTests/inspector/g... LayoutTests/inspector/geolocation-emulation-tests.html:44: InspectorTest.addConsoleSniffer(next); I don't understand how this sniffer is used. It will still call next after the first call to "overrideGeolocation()" it is unclear to me what you are trying to achieve. Do you want to set up this sniffer before each overrideGeolocation() call and not pass it into evaluateInPage?
On 2014/02/24 09:41:36, pfeldman wrote: > https://codereview.chromium.org/172393006/diff/130001/LayoutTests/inspector/g... > File LayoutTests/inspector/geolocation-emulation-tests.html (right): > > https://codereview.chromium.org/172393006/diff/130001/LayoutTests/inspector/g... > LayoutTests/inspector/geolocation-emulation-tests.html:44: > InspectorTest.addConsoleSniffer(next); > I don't understand how this sniffer is used. It will still call next after the > first call to "overrideGeolocation()" it is unclear to me what you are trying to > achieve. Do you want to set up this sniffer before each overrideGeolocation() > call and not pass it into evaluateInPage? You are right, I did not use addConsoleSniffer at all. I did not realize testInvalidParam was not calling "next" because of exception. I have removed sniffer code in above patch. Please take a look.
https://codereview.chromium.org/172393006/diff/190002/LayoutTests/inspector/g... File LayoutTests/inspector/geolocation-emulation-tests.html (right): https://codereview.chromium.org/172393006/diff/190002/LayoutTests/inspector/g... LayoutTests/inspector/geolocation-emulation-tests.html:63: InspectorTest.evaluateInPage("overrideGeolocation()", next); There is no guarantee that testSuccess/testFailed will be called before you call next here and hence you might end the test before geolocation is printed to the console. It sounds like you need to use sniffer for each test transition.
On 2014/02/25 16:23:29, pfeldman wrote: > https://codereview.chromium.org/172393006/diff/190002/LayoutTests/inspector/g... > File LayoutTests/inspector/geolocation-emulation-tests.html (right): > > https://codereview.chromium.org/172393006/diff/190002/LayoutTests/inspector/g... > LayoutTests/inspector/geolocation-emulation-tests.html:63: > InspectorTest.evaluateInPage("overrideGeolocation()", next); > There is no guarantee that testSuccess/testFailed will be called before you call > next here and hence you might end the test before geolocation is printed to the > console. It sounds like you need to use sniffer for each test transition. OK and sniffer will execute next the moment something is written to console? Either testSuccess or testFailure? I will update patch to use sniffer. If you are around for 10-15 mins I can upload it right away, I actually also have a blocking CL https://codereview.chromium.org/143003024/ to update once I am through with this CL. Thanks for reviews!
Please take a look. I have added sniffer code back
lgtm!
On 2014/02/25 16:59:18, pfeldman wrote: > lgtm! Thanks pfeldman!
The CQ bit was checked by mahesh.kk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/172393006/240001
Message was sent while issue was closed.
Change committed as 167810 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
