Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(108)

Issue 2529043002: [CSS Typed OM] Add a property test generator for testing the inline StylePropertyMap (Closed)

Created:
4 years ago by meade_UTC10
Modified:
4 years ago
Reviewers:
dstockwell, shans, qyearsley
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews, rjwright
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a property test generator for testing the inline StylePropertyMap This will allow easier testing of each property as we add support in CSS Typed OM, as we will not have to hand-write exhaustive tests for each property, and we will easily be able to add a new test that applies to every element easily if we need to later. The standard set of tests is: *single-valued properties* - Setting with a sequence of valid values throws - Calling append with a valid value throws *list-valued properties* - Setting with a sequence of valid values works - Setting with a sequence containing valid and invalid values throws - Append succeeds with a valid value - Appending a sequence of valid values succeeds - Append throws when given an invalid value - Append throws when given a sequence containing valid and invalid values - GetAll when the property is set to a list returns all the properties *common* - Can be set to each valid keyword (automatically adds initial, inherit and unset) - Can be set to each given valid instance of StyleValue - Setting to an invalid type throws (automatically adds null, undefined, true, false, 1, 'hello', {}, and a fake keyword) - Get returns the right KeywordValue when the property is set to each valid keyword - Get returns the right StyleValue when the property is set to .cssText of each given valid StyleValue - GetAll works when the property is set to a valid StyleValue instance - Delete removes the style from the element - GetProperties includes the name of the property when it is set to something I've added example tests for "bottom" and "animation-direction", as semi-random, convenient single-valued and list-valued properties. Note that since getting CSSKeyword from a StylePropertyMap is not yet fully supported, the new tests fail. BUG=545318 Committed: https://crrev.com/09bfd57f7db4f626a19d29e3ee9ca6d1ae3eb1dd Cr-Commit-Position: refs/heads/master@{#435569}

Patch Set 1 #

Patch Set 2 : Simplify contents of CL #

Patch Set 3 : Update test names and add list-supporting property test #

Patch Set 4 : Use animation-direction instead of animation-iteration-count for a list-valued test #

Patch Set 5 : Add expectation files #

Total comments: 22

Patch Set 6 : Address CL comments #

Patch Set 7 : update to use for..of and let #

Patch Set 8 : Fix typo #

Messages

Total messages: 26 (12 generated)
meade_UTC10
4 years ago (2016-11-29 05:37:31 UTC) #6
meade_UTC10
Shane: PTAL, thanks! Renee: FYI
4 years ago (2016-11-29 05:38:43 UTC) #7
shans
lgtm! I really like this approach - it looks like it's going to exhaustively test ...
4 years ago (2016-11-29 05:59:59 UTC) #8
meade_UTC10
Thanks Shane! Doug, PTAL for OWNERS? Thank you!
4 years ago (2016-11-29 06:04:34 UTC) #10
dstockwell
lgtm https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html#newcode6 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html:6: <div id="testElement"></div> The suite should just generate this ...
4 years ago (2016-11-29 06:26:35 UTC) #11
dstockwell
lgtm https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html#newcode6 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html:6: <div id="testElement"></div> The suite should just generate this ...
4 years ago (2016-11-29 06:26:35 UTC) #12
shans
https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js#newcode121 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js:121: assert_equals(element.style[propertyName], validObject.cssText + ' ' + validObject.cssText); BTW shouldn't ...
4 years ago (2016-11-29 20:58:14 UTC) #13
qyearsley
https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js#newcode3 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js:3: * inline StylePropertyMap. Might be helpful to add a ...
4 years ago (2016-11-30 18:37:24 UTC) #15
meade_UTC10
https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html#newcode6 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/animation-direction.html:6: <div id="testElement"></div> On 2016/11/29 06:26:35, dstockwell wrote: > The ...
4 years ago (2016-12-01 03:48:21 UTC) #16
meade_UTC10
https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/Source/core/css/CSSProperties.in File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/Source/core/css/CSSProperties.in#newcode122 third_party/WebKit/Source/core/css/CSSProperties.in:122: animation-direction keywords=[normal|reverse|alternate|alternate-reverse], supports_multiple, custom_all On 2016/11/29 06:26:35, dstockwell wrote: ...
4 years ago (2016-12-01 03:50:42 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2529043002/140001
4 years ago (2016-12-01 05:50:27 UTC) #20
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years ago (2016-12-01 06:58:46 UTC) #23
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/09bfd57f7db4f626a19d29e3ee9ca6d1ae3eb1dd Cr-Commit-Position: refs/heads/master@{#435569}
4 years ago (2016-12-01 07:03:31 UTC) #25
qyearsley
4 years ago (2016-12-01 16:39:44 UTC) #26
Message was sent while issue was closed.
https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js
(right):

https://codereview.chromium.org/2529043002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/property-suite.js:24:
function generatePropertySuite(propertyName, config, element) {
On 2016/12/01 at 03:48:20, Eddy wrote:
> On 2016/11/30 18:37:24, qyearsley (OOO until Nov 30) wrote:
> > Arguably, these functions could also be named as "test*" instead of
"generate*",
> > since they invoke test() and don't return anything, e.g.
> > 
> >   testPropertySuite
> >   testSet
> >   testGet
> >   testGetAll
> >   testDelete
> >   testGetProperties
> >   testSetSequence
> >   testAppend
> >   testMultipleValuesNotSupported
> > 
> > Although "generate" is also fine.
> 
> I had actually renamed these to "runFooTests" but haven't uploaded...

That sounds good to me too :-)

Powered by Google App Engine
This is Rietveld 408576698