Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(86)

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

Created:
3 years, 11 months ago by meade_UTC10
Modified:
3 years, 11 months 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
3 years, 11 months ago (2016-11-29 05:37:31 UTC) #6
meade_UTC10
Shane: PTAL, thanks! Renee: FYI
3 years, 11 months 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 ...
3 years, 11 months ago (2016-11-29 05:59:59 UTC) #8
meade_UTC10
Thanks Shane! Doug, PTAL for OWNERS? Thank you!
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 11 months 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: ...
3 years, 11 months 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
3 years, 11 months ago (2016-12-01 05:50:27 UTC) #20
commit-bot: I haz the power
Committed patchset #8 (id:140001)
3 years, 11 months 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}
3 years, 11 months ago (2016-12-01 07:03:31 UTC) #25
qyearsley
3 years, 11 months 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