Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2823163002:
Port some AOM tests from LayoutTests to SimTests. (Closed)
DescriptionPort some AOM tests from LayoutTests to SimTests.
BUG=680345
Review-Url: https://codereview.chromium.org/2823163002
Cr-Commit-Position: refs/heads/master@{#466272}
Committed: https://chromium.googlesource.com/chromium/src/+/45494c0941e362c28b04b92f237c759f8c960794
Patch Set 1 #
Total comments: 20
Patch Set 2 : Remove calls to ExecuteJavaScript #
Total comments: 2
Patch Set 3 : Address last feedback #
Messages
Total messages: 27 (14 generated)
The CQ bit was checked by dmazzoni@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...
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org, esprehn@chromium.org
https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:14: class AccessibilityObjectModelTest : public SimTest { If this is ported, what was the original?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:14: class AccessibilityObjectModelTest : public SimTest { On 2017/04/18 05:43:16, aboxhall wrote: > If this is ported, what was the original? The three tests here just duplicate existing tests from: LayoutTests/accessibility/aom.html I don't want to delete the layout tests, but the plan is to switch to preferring SimTests, while maintaining existing layout tests and still using them on occasion when they're a better fit for a particular test.
lgtm
dmazzoni@chromium.org changed reviewers: + bokan@chromium.org
@bokan, I'd appreciate it if you could give me a secondary review since this is my first time working with SimTests. Any feedback would be great, I'm hoping to set up some good patterns to follow before adding several more tests. Thanks!
This lgtm but I'm new to SimTests myself, esprehn@ is the expert here. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:31: RuntimeEnabledFeatures::setAccessibilityObjectModelEnabled(true); You already set this to true in the constructor, right? Also, I think you should prefer using ScopedRuntimeEnabledFeatureForTest (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi...)
https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:30: main_resource.Complete("<button id=\"button\">Click me</button>"); id=button, no quotes are needed and it avoids the escaping https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:33: String result = ExecuteJavaScript( Just do this in C++, auto* button = GetDocument().getElementById("button"); EXPECT_NE(nullptr, button->accesibleNode()); https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:41: main_resource.Complete("<button id=\"button\">Click me</button>"); the button is not needed, just do "Text node" ? https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:42: RuntimeEnabledFeatures::setAccessibilityObjectModelEnabled(true); not needed https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:44: String result = ExecuteJavaScript( Is there a reason not to just do auto* text = GetDocument().createTextNode("example"); EXPECT_EQ(nullptr, text->accessibleNode()); ? https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:56: "document.getElementById('button').accessibleNode.role = 'slider';"); auto* button = GetDocument().getElementById("button"); button->accessibleNode()->setRole("slider") https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:59: EXPECT_TRUE(button); ASSERT_NE(nullptr, button) the code would crash if this condition was false, so it's ASSERT not EXPECT, also a more explicit comparison is usually preferred https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/sim/SimTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/sim/SimTest.cpp:100: WebView().MainFrame()->ToWebLocalFrame()->RequestExecuteScriptAndReturnValue( Since you can access the core types directly this should be: ...GetScriptController().ExecuteScriptInMainWorld(source) https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/sim/SimTest.h (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/sim/SimTest.h:38: String ExecuteJavaScript(String); The spirit of sim tests is that you should avoid writing script in general. I'd prefer not to add this.
Got it, so these should be pure unit tests and not even touch the JavaScript bindings. It sounds like we should keep some layout tests just to test the bindings and demonstrate usage of the API, but use the SimTests for robust code coverage because of their speed. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:30: main_resource.Complete("<button id=\"button\">Click me</button>"); On 2017/04/20 19:54:06, esprehn wrote: > id=button, no quotes are needed and it avoids the escaping Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:31: RuntimeEnabledFeatures::setAccessibilityObjectModelEnabled(true); On 2017/04/20 14:58:33, bokan wrote: > You already set this to true in the constructor, right? Also, I think you should > prefer using ScopedRuntimeEnabledFeatureForTest > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi...) Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:33: String result = ExecuteJavaScript( On 2017/04/20 19:54:06, esprehn wrote: > Just do this in C++, > > auto* button = GetDocument().getElementById("button"); > EXPECT_NE(nullptr, button->accesibleNode()); Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:41: main_resource.Complete("<button id=\"button\">Click me</button>"); On 2017/04/20 19:54:06, esprehn wrote: > the button is not needed, just do "Text node" ? Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:42: RuntimeEnabledFeatures::setAccessibilityObjectModelEnabled(true); On 2017/04/20 19:54:06, esprehn wrote: > not needed Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:44: String result = ExecuteJavaScript( On 2017/04/20 19:54:06, esprehn wrote: > Is there a reason not to just do > > auto* text = GetDocument().createTextNode("example"); > EXPECT_EQ(nullptr, text->accessibleNode()); > > ? Because accessibleNode is not even defined on Node? I'll just delete this test. It's only useful as a sanity test for the bindings. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:56: "document.getElementById('button').accessibleNode.role = 'slider';"); On 2017/04/20 19:54:06, esprehn wrote: > auto* button = GetDocument().getElementById("button"); > button->accessibleNode()->setRole("slider") Done. https://codereview.chromium.org/2823163002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:59: EXPECT_TRUE(button); On 2017/04/20 19:54:06, esprehn wrote: > ASSERT_NE(nullptr, button) > > the code would crash if this condition was false, so it's ASSERT not EXPECT, > also a more explicit comparison is usually preferred Done.
The CQ bit was checked by dmazzoni@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2823163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:36: EXPECT_NE(nullptr, button->accessibleNode()); Could you assert about some properties of the AOM object too? Might be nice
On 2017/04/21 at 00:07:35, dmazzoni wrote: > Got it, so these should be pure unit tests and not even > touch the JavaScript bindings. > > It sounds like we should keep some layout tests just to > test the bindings and demonstrate usage of the API, > but use the SimTests for robust code coverage because > of their speed. Yeah the "spirit" of SimTest is that you write C++ like you would JS, but avoid running actual JS as much as possible. We should evolve the C++ internal APIs to be easier to use in this manner whenever possible too. That does mean you need to keep some JS tests around, but overall we should trust web platform tests to exercise the bindings side.
https://codereview.chromium.org/2823163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp (right): https://codereview.chromium.org/2823163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/AccessibilityObjectModelTest.cpp:36: EXPECT_NE(nullptr, button->accessibleNode()); On 2017/04/21 03:14:46, esprehn wrote: > Could you assert about some properties of the AOM object too? Might be nice Sure. I'll follow up with a lot more tests too.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, aboxhall@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2823163002/#ps40001 (title: "Address last feedback")
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": 40001, "attempt_start_ts": 1492750408949920,
"parent_rev": "92daff58d9c46d9ba81d43bb4ddf91bcfe82d3c0", "commit_rev":
"45494c0941e362c28b04b92f237c759f8c960794"}
Message was sent while issue was closed.
Description was changed from ========== Port some AOM tests from LayoutTests to SimTests. BUG=680345 ========== to ========== Port some AOM tests from LayoutTests to SimTests. BUG=680345 Review-Url: https://codereview.chromium.org/2823163002 Cr-Commit-Position: refs/heads/master@{#466272} Committed: https://chromium.googlesource.com/chromium/src/+/45494c0941e362c28b04b92f237c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/45494c0941e362c28b04b92f237c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
