Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908103002:
NetInfo external Web LayoutTest (Closed)
DescriptionNetInfo external Web LayoutTest
The tests verify whether different NetInfo attributes are enabled.
BUG=719108
Review-Url: https://codereview.chromium.org/2908103002
Cr-Commit-Position: refs/heads/master@{#477415}
Committed: https://chromium.googlesource.com/chromium/src/+/b11d7ccc96b8092074b85b884337a4bfe7e7723c
Patch Set 1 : ps #
Total comments: 10
Patch Set 2 : foolip comments #
Total comments: 4
Patch Set 3 : foolip comments: merge back to single file #
Total comments: 1
Messages
Total messages: 45 (32 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== NetInfo BUG= ========== to ========== NetInfo external Web LayoutTest BUG= ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== NetInfo external Web LayoutTest BUG= ========== to ========== NetInfo external Web LayoutTest The tests verify whether different NetInfo attributes are enabled. BUG= ==========
Patchset #1 (id:60001) has been deleted
Description was changed from ========== NetInfo external Web LayoutTest The tests verify whether different NetInfo attributes are enabled. BUG= ========== to ========== NetInfo external Web LayoutTest The tests verify whether different NetInfo attributes are enabled. BUG=719108 ==========
tbansal@chromium.org changed reviewers: + foolip@chromium.org
foolip: ptal. Thanks.
wpt, yay! Some nits, happy to take a second look after. If you feel like it, and idlharness.js test might also come in handy. https://codereview.chromium.org/2862423002 is a pretty straightforward example to follow if you want to do that. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html (right): https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:5: <meta name="flags" content=""> This line isn't necessary. Since the filename isn't in the diff, also using this place to suggest naming this netinfo-basics.html or something similar, since presumably there will be other tests eventually to disambiguate this from. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:9: <h2>Description</h2> These bits, while often found in LayoutTests, aren't necessary in web-platform-tests. If you like you can put extra information in comments, or in the title itself. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:16: test(function() { It's a good idea in general to have separate tests if it's plausible that one might fail some assertions but still usefully pass some others, which would be the case if someone implements, say, rtt/downlink but no other attributes. One test for attribute would probably make the most sense, and then it's OK to also omit the assert descriptions if they end up being almost the same as the test title. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:20: assert_true(navigator.connection.type == "bluetooth" assert_in_array would help for this and effectiveType. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:31: assert_greater_than_equal(navigator.connection.rtt, 0, Can you also assert_equals(navigator.connection.rtt % 25, 0) and similar for downlink?
On 2017/05/29 19:28:00, foolip wrote: > wpt, yay! Some nits, happy to take a second look after. > > If you feel like it, and idlharness.js test might also come in handy. > https://codereview.chromium.org/2862423002 is a pretty straightforward example > to follow if you want to do that. Can you expand a bit more on what's the difference between the tests in this CL and idlharness.js based tests?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
The CQ bit was unchecked by tbansal@chromium.org
foolip: ptal. Thanks. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html (right): https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:5: <meta name="flags" content=""> On 2017/05/29 19:28:00, foolip wrote: > This line isn't necessary. > > Since the filename isn't in the diff, also using this place to suggest naming > this netinfo-basics.html or something similar, since presumably there will be > other tests eventually to disambiguate this from. Done. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:9: <h2>Description</h2> On 2017/05/29 19:28:00, foolip wrote: > These bits, while often found in LayoutTests, aren't necessary in > web-platform-tests. If you like you can put extra information in comments, or in > the title itself. Done. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:16: test(function() { On 2017/05/29 19:28:00, foolip wrote: > It's a good idea in general to have separate tests if it's plausible that one > might fail some assertions but still usefully pass some others, which would be > the case if someone implements, say, rtt/downlink but no other attributes. > > One test for attribute would probably make the most sense, and then it's OK to > also omit the assert descriptions if they end up being almost the same as the > test title. Done. Using one test per attribute. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:20: assert_true(navigator.connection.type == "bluetooth" On 2017/05/29 19:28:00, foolip wrote: > assert_in_array would help for this and effectiveType. Thanks, this makes the test much cleaner. https://codereview.chromium.org/2908103002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/netinfo/Netinfo.html:31: assert_greater_than_equal(navigator.connection.rtt, 0, On 2017/05/29 19:28:00, foolip wrote: > Can you also assert_equals(navigator.connection.rtt % 25, 0) and similar for > downlink? Done.
On 2017/05/30 21:48:50, tbansal1 wrote: > On 2017/05/29 19:28:00, foolip wrote: > > wpt, yay! Some nits, happy to take a second look after. > > > > If you feel like it, and idlharness.js test might also come in handy. > > https://codereview.chromium.org/2862423002 is a pretty straightforward example > > to follow if you want to do that. > > Can you expand a bit more on what's the difference between the tests > in this CL and idlharness.js based tests? Also, note that I have https://codereview.chromium.org/2903493002/ in-flight which adds internal layout tests specific to Chromium.
Note that it's fine to put many tests in the same file, as long as the titles are unique. That'll make the tests run a bit faster. pass and failure is on a per-test level and not a per-file level, so my earlier concern about partial implementations is compatible with a single file. LGTM as-is or merged back into a single file, whichever you prefer. (I prefer single.) https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html (right): https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html:3: <title>NetInfo Test</title> If the test itself doesn't have a title (after the function) then this will be used. Can you make the title something that includes "downlink"? (Omitting "test" and similar is fine.) https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html:10: assert_greater_than_equal(navigator.connection.downlink, 0, ''); An empty description won't do anything, just omit it.
On 2017/05/30 21:48:50, tbansal1 wrote: > On 2017/05/29 19:28:00, foolip wrote: > > wpt, yay! Some nits, happy to take a second look after. > > > > If you feel like it, and idlharness.js test might also come in handy. > > https://codereview.chromium.org/2862423002 is a pretty straightforward example > > to follow if you want to do that. > > Can you expand a bit more on what's the difference between the tests > in this CL and idlharness.js based tests? Sure. idlharness.js tests are very surface-level and will generally only exercise Web IDL semantics, not things that are part of the definitions of methods and attributes, because it can't make any assumptions about what will happen when methods are invoked, or when attributes are read/modified. They're still useful though, because they'll catch when something is on the wrong interface (maybe HTMLElement instead of Element), some extended attributes like [LenientSetter], and of course also they'll tell you if something is missing.
On 2017/05/31 14:25:52, foolip wrote: > On 2017/05/30 21:48:50, tbansal1 wrote: > > On 2017/05/29 19:28:00, foolip wrote: > > > wpt, yay! Some nits, happy to take a second look after. > > > > > > If you feel like it, and idlharness.js test might also come in handy. > > > https://codereview.chromium.org/2862423002 is a pretty straightforward > example > > > to follow if you want to do that. > > > > Can you expand a bit more on what's the difference between the tests > > in this CL and idlharness.js based tests? > > Sure. idlharness.js tests are very surface-level and will generally only > exercise Web IDL semantics, not things that are part of the definitions of > methods and attributes, because it can't make any assumptions about what will > happen when methods are invoked, or when attributes are read/modified. > > They're still useful though, because they'll catch when something is on the > wrong interface (maybe HTMLElement instead of Element), some extended attributes > like [LenientSetter], and of course also they'll tell you if something is > missing. One specific thing that the tests in the CL doesn't test but idlharness.js would is that the attributes are readonly. Writing tests that try to set readonly attributes only to check that it doesn't work is rather boring to do manually.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
On 2017/05/31 14:18:14, foolip wrote: > Note that it's fine to put many tests in the same file, as long as the titles > are unique. That'll make the tests run a bit faster. pass and failure is on a > per-test level and not a per-file level, so my earlier concern about partial > implementations is compatible with a single file. > > LGTM as-is or merged back into a single file, whichever you prefer. (I prefer > single.) I have merged them back in the same file. Thanks for the helpful detailed comment.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html (right): https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html:3: <title>NetInfo Test</title> On 2017/05/31 14:18:14, foolip wrote: > If the test itself doesn't have a title (after the function) then this will be > used. Can you make the title something that includes "downlink"? (Omitting > "test" and similar is fine.) Done. https://codereview.chromium.org/2908103002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-downlink.html:10: assert_greater_than_equal(navigator.connection.downlink, 0, ''); On 2017/05/31 14:18:14, foolip wrote: > An empty description won't do anything, just omit it. Done. I have removed the description from places where it was not providing any value over the existing assert condition.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2908103002/#ps120001 (title: "foolip comments: merge back to single file")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496784535766930,
"parent_rev": "cea92c51ac31ffeb3f7c36ade3d56b6fcaf593b5", "commit_rev":
"b11d7ccc96b8092074b85b884337a4bfe7e7723c"}
Message was sent while issue was closed.
Description was changed from ========== NetInfo external Web LayoutTest The tests verify whether different NetInfo attributes are enabled. BUG=719108 ========== to ========== NetInfo external Web LayoutTest The tests verify whether different NetInfo attributes are enabled. BUG=719108 Review-Url: https://codereview.chromium.org/2908103002 Cr-Commit-Position: refs/heads/master@{#477415} Committed: https://chromium.googlesource.com/chromium/src/+/b11d7ccc96b8092074b85b884337... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b11d7ccc96b8092074b85b884337...
Message was sent while issue was closed.
https://codereview.chromium.org/2908103002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-basics.html (right): https://codereview.chromium.org/2908103002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/netinfo/netinfo-basics.html:12: }); This is now live at http://w3c-test.org/netinfo/netinfo-basics.html and as you can tell, the test titles are generated from <title>. You can also add titles here, after the function for each test. This is an FYI, no need to follow up if you don't think more specific titles would be useful. |
