Rewrite css/cssom-1/index-003 from generate_tests syntax #5263

melanierichards · 2017-03-29T21:50:02Z

In an effort to help @gsnedders bring the CSS tests into the fold with the current syntax, this PR removes the generate_tests() syntax from css/cssom-1/index-003.html. In order to improve clarity for future testers, the old test has been split into two files named for the two concepts being tested: cssom-ruleOrder.html and cssom-ruleType.html. There are perhaps additional updates that can be made to these tests, but the main goal of this PR is to remove generate_tests().

wpt-pr-bot · 2017-03-29T21:50:11Z

Notifying @dbaron, @lilles, @plinss, @rune-opera, @therealglazou, and @zcorpan. (Learn how reviewing works.)

ghost · 2017-03-29T23:07:05Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 1b4e7ca
Using browser at version BuildID 20170405115424; SourceStamp b043233ec04f06768d59dcdfb9e928142280f3cc
Starting 10 test iterations

ghost · 2017-03-29T23:07:06Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 1b4e7ca
Using browser at version 59.0.3053.3 dev
Starting 10 test iterations

gsnedders · 2017-04-03T22:20:20Z

w3c-test:mirror

gsnedders · 2017-04-03T21:55:55Z

css/cssom-1/cssom-ruleOrder.html

@@ -16,6 +17,7 @@
  <style id="s-2">
    h1 { background: indianred; }
  </style>
+  <!-- Unsure why the second of 2 rules is the test subject in #s-3 -->


What are you unsure about here? To test namespace selectors it needs to define the namespace, hence if it wants to test how a namespace selector is serialised it needs a rule before it.

Regardless, the comment should go. Maybe with an explanation somewhere. Or both here and in the test function.

The svg namespace is already defined in stylesheet #s-0, was thinking that it didn't need to be declared again. But this may be my test-writing-newness showing, perhaps it needs to be directly in this stylesheet to be accurately tested?

gsnedders · 2017-04-03T22:08:12Z

css/cssom-1/cssom-ruleOrder.html

+   for (var i = 0; i < stylesheets.length; i++) {
+    test( function () {
+        if (i === 3) {
+          var cssType = stylesheets[i].cssRules.length > 0 && stylesheets[i].cssRules[1] ? stylesheets[i].cssRules[1].type : 11;


This seems overly complex (I realise this is pre-existing!); can it not just be stylesheets[i].cssRules.length > 1 ? stylesheets[i].cssRules[1].type : 11?

Actually, I'm not sure why we have the conditional operator there at all. It just seems to be providing a default value so it has something to push to the old results array and the check for the length is only needed to make sure it doesn't throw in case it fails (which we need in the generate_tests case because otherwise it won't run any tests, which is the whole reason why it is far too fragile).

I think we can actually literally just do stylesheets[i].cssRules[1].type (and similar, but 0, below), and accept that'll throw if for some reason we don't get the cssRules we expect, but that'll be caught by the test and then it doesn't hide that problem.

As such, this seems like simplification worth doing while getting rid of generate_tests.

gsnedders · 2017-04-03T22:18:09Z

css/cssom-1/cssom-ruleType.html

+    };
+
+    for (var i = 0; i < stylesheets.length; i++) {
+      test( function () {


This seems entirely identical to the test in cssom-ruleOrder.html? Apart from renaming expectedRuleOrder to expectedTypes and changing the test name, it seems like nothing has changed here? As such, they're not actually testing anything different?

I don't really know what it's meant to be doing with rule order in the other, so should that be doing something else?

I can see a couple paths forward here. The original test is looking at 2 different things: 1) that the stylesheets are applied in the correct order and 2) that the evaluated types are correct. However, to test ORDER, it's actually checking to see if the evaluated TYPES are in the expected order, given the rules in the stylesheets. I think there could be a better way to test order, but that would be out of the scope of this particular PR.

Knowing this, the 2 options forward in my mind:

Keep the tests separate, and in future find a better way to test order (maybe check to see that .cssText returns the expected rule?)

Recombine and not worry too much about update the rule order functionality; other fish to fry etc.

@gsnedders thoughts?

@melanierichards However, the two things that are being tested essentially end up being tested in equivalent ways: to test the order, it's looking at the types; to test the types, it's looking at the order. As such, it seems kinda silly to make them separate. (I agree there's probably a better way but that's out-of-scope.)

gsnedders · 2017-04-03T22:19:04Z

css/cssom-1/cssom-ruleOrder.html

+    var expectedRuleOrder = [10, 3, 1, 1, 5, 4, 6];
+
+    var typesText = {
+      1: 'style Rule',


Should be style rule, all lowercase

gsnedders · 2017-04-03T22:26:28Z

css/cssom-1/cssom-ruleOrder.html

  <link rel="author" title="Divya Manian" href="mailto:manian@adobe.com">
+  <link rel="author" title="Melanie Richards" href="mailto:Melanie.Richards@microsoft.com">


No need to maintain author links (we have them all in git history, except in the case of old contributors who didn't have VCS access when the CSS testsuite was in CVS/SVN), and we have Divya as the original author in the file's history, so you're just as well off deleting it.

ghost · 2017-04-06T17:14:02Z

These tests are now available on w3c-test.org

gsnedders · 2017-04-06T17:50:05Z

The Chrome failure on Travis is #5407; the long-running Firefox one possibly-going-to-timeout one is #5408.

zcorpan · 2017-04-10T11:39:32Z

I suppose this now needs a rebase to move to /cssom/

ghost · 2017-05-25T21:39:02Z

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 63afe441ce6f237c3eed0efdcfcdc41fc44b2070
Using browser at version BuildID 20170530100155; SourceStamp 286f71223256cbb3a769432fd860f563c4886e81
Starting 10 test iterations
All results were stable

All results

1 test ran

/cssom/cssom-ruleTypeAndOrder.html

Subtest	Results	Messages
	OK
`Type of #s-0 is expected to be namespace rule`	PASS
`Type of #s-1 is expected to be @import rule`	PASS
`Type of #s-2 is expected to be style rule`	PASS
`Type of #s-3 is expected to be style rule`	PASS
`Type of #s-4 is expected to be @font-face rule`	PASS
`Type of #s-5 is expected to be media rule`	PASS
`Type of #s-6 is expected to be page rule`	FAIL	`stylesheets[i].cssRules[0] is undefined`

ghost · 2017-05-25T21:39:03Z

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision bd8e248
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran

/cssom/cssom-ruleTypeAndOrder.html

Subtest	Results	Messages
	OK
`Type of #s-0 is expected to be namespace rule`	FAIL	`undefined is not an object (evaluating 'stylesheets[i].cssRules[0].type')`
`Type of #s-1 is expected to be @import rule`	PASS
`Type of #s-2 is expected to be style rule`	PASS
`Type of #s-3 is expected to be style rule`	FAIL	`undefined is not an object (evaluating 'stylesheets[i].cssRules[1].type')`
`Type of #s-4 is expected to be @font-face rule`	PASS
`Type of #s-5 is expected to be media rule`	PASS
`Type of #s-6 is expected to be page rule`	PASS

ghost · 2017-05-25T21:39:04Z

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision bd8e248
Using browser at version 60.0.3112.7 dev
Starting 10 test iterations
All results were stable

All results

1 test ran

/cssom/cssom-ruleTypeAndOrder.html

Subtest	Results	Messages
	OK
`Type of #s-0 is expected to be namespace rule`	PASS
`Type of #s-1 is expected to be @import rule`	PASS
`Type of #s-2 is expected to be style rule`	PASS
`Type of #s-3 is expected to be style rule`	PASS
`Type of #s-4 is expected to be @font-face rule`	PASS
`Type of #s-5 is expected to be media rule`	PASS
`Type of #s-6 is expected to be page rule`	PASS

ghost · 2017-05-25T21:39:05Z

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision bd8e248
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran

/cssom/cssom-ruleTypeAndOrder.html

Subtest	Results	Messages
	OK
`Type of #s-0 is expected to be namespace rule`	PASS
`Type of #s-1 is expected to be @import rule`	PASS
`Type of #s-2 is expected to be style rule`	PASS
`Type of #s-3 is expected to be style rule`	PASS
`Type of #s-4 is expected to be @font-face rule`	PASS
`Type of #s-5 is expected to be media rule`	PASS
`Type of #s-6 is expected to be page rule`	PASS

Also splits this test into 2 separate tests: cssom-ruleType and -ruleOrder Syntax changes to rule order and rule type based on feedback

melanierichards · 2017-05-26T23:33:41Z

@gsnedders sigh, it looks like every time I sync this branch it's just going to push all those changes back in again. I'm thinking maybe we rebase it (or whatever you did to clean things up) once we're all set on changes. This should be the only file that matters, really: https://github.com/w3c/web-platform-tests/pull/5263/files#diff-7dec3f0dbdac0fbf5b02bee5f3b1d0e9

gsnedders · 2017-05-30T15:18:22Z

@melanierichards You seem to have merged in the remote branch into yours instead of resetting to it.

Try the following, with the branch checked out:

git reset --hard f6ff583c2350c16267bd02da7d521bb343483c94
git cherry-pick e49b4300aaff8a7bb19941e3529f03f585ccf0cb
git push --force-with-lease

Basically: reset the branch to point at the tidied up version from last week, copy in your commit from Friday, and push that cleaned up branch with only those commits to GitHub.

melanierichards · 2017-05-30T22:05:19Z

✨ Diffs are now squeaky clean ✨

Thanks for the help, @gsnedders!

Comments are addressed

zcorpan · 2017-05-31T07:47:06Z

Thank you! ❤️

Also splits this test into 2 separate tests: cssom-ruleType and -ruleOrder

gsnedders · 2017-08-02T10:36:12Z

I'm assuming there's no reason why @zcorpan didn't merge this before (waiting for CI from his commit?).

wpt-pr-bot added wg-css cssom-1 labels Mar 29, 2017

gsnedders self-requested a review March 29, 2017 22:23

zcorpan self-requested a review March 30, 2017 15:04

gsnedders previously requested changes Apr 3, 2017

View reviewed changes

gsnedders mentioned this pull request Apr 5, 2017

Merge css/cssom-1/ and css/cssom-view-1/ into cssom/ and cssom-view/ #5269

Merged

wpt-pr-bot added the infra label May 25, 2017

wpt-pr-bot requested a review from jgraham May 25, 2017 21:31

Rewrite css/cssom-1/index-003 from generate_tests syntax

f6ff583

Also splits this test into 2 separate tests: cssom-ruleType and -ruleOrder Syntax changes to rule order and rule type based on feedback

gsnedders force-pushed the cssom1-003-remove-generate-tests branch from 0be66c5 to f6ff583 Compare May 26, 2017 22:14

wpt-pr-bot added the cssom label May 26, 2017

wpt-pr-bot requested review from dbaron, lilles, therealglazou, plinss and a user May 26, 2017 22:14

Recombine CSSOM rule order and type

1df2d68

melanierichards force-pushed the cssom1-003-remove-generate-tests branch from e49b430 to 1df2d68 Compare May 30, 2017 22:03

Link to editor's draft

fffe04b

zcorpan approved these changes May 31, 2017

View reviewed changes

melanierichards referenced this pull request in melanierichards/web-platform-tests Jun 14, 2017

Rewrite css/cssom-1/index-003 from generate_tests syntax

317dc5f

Also splits this test into 2 separate tests: cssom-ruleType and -ruleOrder

gsnedders merged commit c147d34 into web-platform-tests:master Aug 2, 2017

gsnedders removed the cssom-1 label Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite css/cssom-1/index-003 from generate_tests syntax #5263

Rewrite css/cssom-1/index-003 from generate_tests syntax #5263

melanierichards commented Mar 29, 2017 •

edited by wpt-pr-bot

Loading

wpt-pr-bot commented Mar 29, 2017

ghost commented Mar 29, 2017 •

edited by ghost

Loading

ghost commented Mar 29, 2017 •

edited by ghost

Loading

gsnedders commented Apr 3, 2017

gsnedders Apr 3, 2017

gsnedders Apr 3, 2017

melanierichards Apr 6, 2017 •

edited

Loading

gsnedders Apr 3, 2017

gsnedders Apr 3, 2017

melanierichards Apr 6, 2017

gsnedders May 10, 2017

melanierichards May 10, 2017

gsnedders Apr 3, 2017

gsnedders Apr 3, 2017

ghost commented Apr 6, 2017

gsnedders commented Apr 6, 2017 •

edited

Loading

zcorpan commented Apr 10, 2017

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

melanierichards commented May 26, 2017

gsnedders commented May 30, 2017

melanierichards commented May 30, 2017

zcorpan commented May 31, 2017

gsnedders commented Aug 2, 2017

		<link rel="author" title="Divya Manian" href="mailto:manian@adobe.com">
		<link rel="author" title="Melanie Richards" href="mailto:Melanie.Richards@microsoft.com">

Rewrite css/cssom-1/index-003 from generate_tests syntax #5263

Rewrite css/cssom-1/index-003 from generate_tests syntax #5263

Conversation

melanierichards commented Mar 29, 2017 • edited by wpt-pr-bot Loading

wpt-pr-bot commented Mar 29, 2017

ghost commented Mar 29, 2017 • edited by ghost Loading

Firefox (nightly channel)

ghost commented Mar 29, 2017 • edited by ghost Loading

Chrome (unstable channel)

gsnedders commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melanierichards Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 6, 2017

gsnedders commented Apr 6, 2017 • edited Loading

zcorpan commented Apr 10, 2017

ghost commented May 25, 2017 • edited by ghost Loading

Firefox (nightly)

All results

ghost commented May 25, 2017 • edited by ghost Loading

Sauce (safari)

All results

ghost commented May 25, 2017 • edited by ghost Loading

Chrome (unstable)

All results

ghost commented May 25, 2017 • edited by ghost Loading

Sauce (MicrosoftEdge)

All results

melanierichards commented May 26, 2017

gsnedders commented May 30, 2017

melanierichards commented May 30, 2017

zcorpan commented May 31, 2017

gsnedders commented Aug 2, 2017

melanierichards commented Mar 29, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Mar 29, 2017 •

edited by ghost

Loading

ghost commented Mar 29, 2017 •

edited by ghost

Loading

melanierichards Apr 6, 2017 •

edited

Loading

gsnedders commented Apr 6, 2017 •

edited

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading

ghost commented May 25, 2017 •

edited by ghost

Loading