Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2089383003:
Added test for step 2, 14 in define element. (Closed)
|
Created:
4 years, 6 months ago by davaajav Modified:
4 years, 5 months ago Reviewers:
dominicc (has gone to gerrit) CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded test for step 2, 14 in define element.
Test for step 2.
- Tests cases when HTMLElement, HTMLButtonElement
is passed as constructor. Step 2 should throw
TypeError, but it passes without errors.
- Passing author-defined custom element constructors
created via class extends HTMLElement {} passes
this step without TypeError.
- Passing JavaScript class Set fails during upgrade.
Test for step 14.
- If Type(prototype) is an Object, and connectedCallback
is not defined and not Callable, it throws TypeError.
- If Type(prototype) is an Object, and disconnectedCallback
is not defined and not Callable, it throws TypeError.
- If attributeChangedCallback is not Callable is not
undefined and not Callable, it throws TypeError.
- If any step in the first set of steps in 14 throws,
it rethrows that exception and terminates the
algorithm.
BUG=594918
Committed: https://crrev.com/9d82f81d395329a5426d4ee459c2006b25da76b5
Cr-Commit-Position: refs/heads/master@{#403862}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Define-element patch updates #Patch Set 3 : added rethrow error tests for step 14 #Patch Set 4 : changed test 14.9.2 from bad to goo #Patch Set 5 : check that Get("observedAttributes") is not executed when attirbuteChangedCallback is undefined #Patch Set 6 : deleted extra line between tests #Patch Set 7 : deleted test where ot checks order of 14.2 and 14.9 it is trivial with the new test #Patch Set 8 : observedAttributes should not be invoked test update #
Total comments: 11
Patch Set 9 : patch update #Patch Set 10 : patch update #
Total comments: 6
Patch Set 11 : added assert-throws-dom-exception #Patch Set 12 : throws DOMEXception NotSupportedError #
Total comments: 1
Patch Set 13 : assert_throws_dom_exception update #Patch Set 14 : assert_throws_dom_exception update #
Total comments: 1
Patch Set 15 : patch update #Messages
Total messages: 24 (6 generated)
davaajav@google.com changed reviewers: + dominicc@chromium.org, dominicc@google.com
PTL
Description was changed from
==========
Added test for step 2, 14 in define element.
Test for step 2.
- Tests cases when HTMLElement, HTMLButtonElement
is passed as cosntructor. Step 2 should throw
TypeError, but it passes without erros.
- Passing author-defined custom element constructors
created via class extends HTMLElement {} passes
this step without TypeError.
- Passing JavaScript class Set fails during upgrade.
Test for step 14.
- If Type(prototype) is an Object, and connectedCallback
is not defined and not Callable, it throws TypeError.
- If Type(prototype) is na Object, and disconnectedCallback
is not defined and not Callable, it throws TypeError.
- If attributeChangedCallback is not Callable is not
undefined and not Callable, it throws Typeerror.
- If any step in the first set of steps in 14 throws,
it rethrows that exception and terminates the
algorithm.
BUG=594918
==========
to
==========
Added test for step 2, 14 in define element.
Test for step 2.
- Tests cases when HTMLElement, HTMLButtonElement
is passed as constructor. Step 2 should throw
TypeError, but it passes without errors.
- Passing author-defined custom element constructors
created via class extends HTMLElement {} passes
this step without TypeError.
- Passing JavaScript class Set fails during upgrade.
Test for step 14.
- If Type(prototype) is an Object, and connectedCallback
is not defined and not Callable, it throws TypeError.
- If Type(prototype) is an Object, and disconnectedCallback
is not defined and not Callable, it throws TypeError.
- If attributeChangedCallback is not Callable is not
undefined and not Callable, it throws TypeError.
- If any step in the first set of steps in 14 throws,
it rethrows that exception and terminates the
algorithm.
BUG=594918
==========
dominicc@chromium.org changed reviewers: - dominicc@google.com
This is very nice work. These tests are very important to custom elements getting implemented correctly in all browsers, so I'm going to give you a lot of feedback so we can make these perfect. It might mean there's a lot of comments in the first few reviews though, don't worry, reviews when you are new to a project are always like this. I've only commented the first time on an issue, so read through the rest of your patch and see if a similar comment applies, and go ahead and make those changes there too. On the whole these tests look really good. Let me know if you have any questions! When you're done upload a new version of your patch and let me know, I'll take a look ASAP. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:67: Delete this blank line; put just one blank line between tests. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:81: assert_true(w.customElements.get('a-a') === X, 'asserting that the first definition worked'); Don't put "asserting" in the message because that's evident from assert_true/assert_false that this is an assertion. Try to incorporate the word "should". Use assert_equals instead of === in cases like this because it will generate a more useful message if the assertion fails (what the result actually was, rather than just "false.") assert_equals does === IIRC; it is exactly what you want. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:82: assert_false(w.customElements.get('a-a') === Y, 'asserting that the first definition worked'); You can probably omit this assertion; it is not very informative given the assertion above. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:202: let invocations = []; There's only one thing going into invocations, so maybe you can simplify the test by eliminating it and using assert_throws. I believe assert_throws has a mode where it does === so you can create and save the object you will throw and then check exactly that was thrown--read the source/docs of assert_throws to be sure. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:215: assert_true(w.customElements.get('a-a') === Y, 'algorithm terminates when the first set of steps threw an exception'); "the first set of steps" is a bit vague. This description doesn't really match what's being tested here; it seems more high-level, that the same name can be registered successfully after failure. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:218: test_with_window((w) => { Tests from here vvv https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:252: }, 'Retrieved attrbuteChangedCallback is not undefined and not callable'); ... to here ^^^ are perfect. Good work. https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:257: assert_throws('SYNTAX_ERR', () => { The implementation has a bug you found--awesome. We have a few choices about what to do in this case. You've written the test so that it passes and matches the (incorrect) implementation. If we upstreamed this test and other browsers wrote code to pass the test, they'd implement the same bug, which is bad. It's like our bad test RNA damaging their DNA. One thing we could do is put a big TODO here saying there's a bug and someone should catch it before we upstream. However the Blink test system also has a way to handle this situation. This is outside of testharness. So you just write the test the way it *should* be (check for TypeError) and the test will fail. Then you can generate this thing called an expectation file by running just this test using run-webkit-tests and passing the --reset-results flag. It will put a file in your source tree like define-element-expected.txt. Add and commit that and upload it as part of this test. The -expected.txt file lets us treat a failing test as an "expected" failure. Then we'll work on making sure we have no -expected.txt files before we ship. You can read more about this here: https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Success-a... https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Script-Tests https://codereview.chromium.org/2089383003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:260: 'should throw TypeError not SyntaxError'); Let's put a comment mentioning the exact step. The technique you're using here, making the first possible successive step throw an error, is really good. But it is subtle to the reader. It's worth a brief comment.
Message was sent while issue was closed.
PTL Added tests for steps 14.1, 14.3, 14.5, 14.7, 14.9.2 and wrapped up the previous ones a bit. Regarding the step 2, "returns Reflect.construct(HTMLElement, [], Foo)" triggers TyperError: Invalid constructor Error
Message was sent while issue was closed.
PTL changed test for 14.9.2 Get(constructor, "observedAttributes") from bad to good.
Message was sent while issue was closed.
Added new test for 14.9.2 checking Get(constructor, "observedAttributes") is not executed when attributeChangedCallback is undefined
Great work, these look excellent. Some more feedback inline. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:104: assert_equals(w.customElements.get('first-name'), X, 'asserting that the first definition worked'); Be consistent; consider using the same description as line 80. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:188: // last statement in 14 Could you add a step number, and maybe some text, here? The spec could change; adding some more clues will make it easier to work out if it has changed. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:227: F.prototype = new Object(); I don't think you need this line; functions get a prototype property with a fresh Object automatically. Below too. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:233: w.customElements.define('a-a', F); Wow, this is clever. I guess callbacks are in the *reverse* order they're retrieved. Could you put a brief comment on the callbacks = line explaining that? If we add callbacks in future, the person editing this file will need to be aware of that. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:234: }, 'Get(prototype, callback) should throw'); If this fails, it will not be clear which one is buggy. Maybe include callbacks[i] in the message? https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:240: let callbacks = ['connectedCallback', 'disconnectedCallback', 'attributeChangedCallback']; Consider moving this outside the test and reusing it. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:255: test_with_window((w) => { As with my comment with the other file, maybe using class syntax is more illustrative and succinct here? https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:280: 'attributeChangedCallback is undefined'); Be consistent with indentation; compare this to line 293. I think the style you have on line 293 where the 's line up is more readable, but at least be consistent. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:291: w.customElements.define(invalid_name, HTMLElement); Indenting JavaScript is surprisingly hard, with arrow functions, promise chains, multiline string literals, etc. In general your style is good. I think in this case, this needs to be indented a couple of spaces to set the body of the arrow function off from the closing brace. Below too. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:311: 'should pass this step without throwing TypeError'); Here's another opportunity to be consistent. You probably want to make a string literal as long as possible before wrapping it, and then choose a good place to wrap it (I try to put the space between words at the *end* of the earlier line.) In this case there's a bit of a visual difference between 301 and 310; 301 is much longer. Go for making this as long is will fit within 80 columns, then wrapping. That way everything will be a consistent length. https://codereview.chromium.org/2089383003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:326: </body> Thanks for adding this; makes sense to add it since we wrote the open tag.
PTL Added new function assert_dom_exception_syntax_error, which is used in "Invalid name" test. Improved comments. Wrap-ups.
Some comments inline. https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (left): https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:51: assert_throws('SYNTAX_ERR', () => { I think you should do assert_dom_exception_syntax_error differently: - Don't make it specific to SYNTAX_ERR, I guess there might be other DOM exceptions we want to test with it? - Put it in custom-elements-helpers.js so we can use it everywhere. - Make it so the caller can just replace assert_throws with assert_throws_dom_exception. You'll need assert_throws_dom_exception to call assert_throws, and pass it a function which wraps the function passed to assert_throws_dom_exception and checks the exception before rethrowing it. https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:220: }, 'Get(constructor, prototype) should throw'); Maybe make this description a bit more specific--it's not that Get(constructor, prototype) should throw as much as the exception from that should be *rethrown*. https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:336: assert_throws('SYNTAX_ERR', () => { This could be another place to use assert_throws_dom_exception. Above too. https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:337: w.customElements.define(invalid_name, class extends HTMLElement {}); Check the indentation here. Above too.
PTL https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:295: }, 'exception thrown while coverting observedAttributes to sequence<DOMString> ' + type coverting https://codereview.chromium.org/2089383003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:318: assert_throws('SYNTAX_ERR', () => { TODO(davaajav): change this to TypeError, when we add a failure expectation to this file
https://codereview.chromium.org/2089383003/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:22: if (e instanceof global_context.DOMException){ Put one space between ) and { With the way you have it here, the assert_throws failing could mean (a) func did not throw an exception or (b) func threw, but not a DOMException. Maybe you could have an assertion here and then rethrow e? I believe assert_throws knows to pass through AssertionError. That way failures will be more specific. WDYT?
PTL I could not come up with a good way to import AssertionError() from testharness.js
PTL
This LGTM, just one small comment inline. https://codereview.chromium.org/2089383003/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2089383003/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:15: // Since testharness cannot test that e instance of the appropriate interface, The grammar of this comment isn't right, Since/because. I'm not sure this needs a comment, really. It has a good name. It uses assert_throws so it is pretty clear from the code what it is doing in addition to assert_throws. Maybe just delete the comment.
PTL
The CQ bit was checked by dominicc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2089383003/#ps280001 (title: "patch update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
Added test for step 2, 14 in define element.
Test for step 2.
- Tests cases when HTMLElement, HTMLButtonElement
is passed as constructor. Step 2 should throw
TypeError, but it passes without errors.
- Passing author-defined custom element constructors
created via class extends HTMLElement {} passes
this step without TypeError.
- Passing JavaScript class Set fails during upgrade.
Test for step 14.
- If Type(prototype) is an Object, and connectedCallback
is not defined and not Callable, it throws TypeError.
- If Type(prototype) is an Object, and disconnectedCallback
is not defined and not Callable, it throws TypeError.
- If attributeChangedCallback is not Callable is not
undefined and not Callable, it throws TypeError.
- If any step in the first set of steps in 14 throws,
it rethrows that exception and terminates the
algorithm.
BUG=594918
==========
to
==========
Added test for step 2, 14 in define element.
Test for step 2.
- Tests cases when HTMLElement, HTMLButtonElement
is passed as constructor. Step 2 should throw
TypeError, but it passes without errors.
- Passing author-defined custom element constructors
created via class extends HTMLElement {} passes
this step without TypeError.
- Passing JavaScript class Set fails during upgrade.
Test for step 14.
- If Type(prototype) is an Object, and connectedCallback
is not defined and not Callable, it throws TypeError.
- If Type(prototype) is an Object, and disconnectedCallback
is not defined and not Callable, it throws TypeError.
- If attributeChangedCallback is not Callable is not
undefined and not Callable, it throws TypeError.
- If any step in the first set of steps in 14 throws,
it rethrows that exception and terminates the
algorithm.
BUG=594918
Committed: https://crrev.com/9d82f81d395329a5426d4ee459c2006b25da76b5
Cr-Commit-Position: refs/heads/master@{#403862}
==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9d82f81d395329a5426d4ee459c2006b25da76b5 Cr-Commit-Position: refs/heads/master@{#403862} |
