Refactor and split up with `variant` all encoding tests #11016

zcorpan · 2018-05-15T21:28:29Z

zcorpan · 2018-05-15T21:29:06Z

I'll apply this to the rest of the encoding tests tomorrow if this seems OK.

inexorabletash · 2018-05-15T21:35:53Z

common/subset-tests.js

+  if (location.search) {
+      match = /^\?(\d+)-(\d+|last)(?:&|$)/.exec(location.search);
+      if (match) {
+        subTestStart = match[1];


nit: parseFloat(match[1]) or parseInt(match[1], 10)

rather than relying on string/number comparison later on. (Of course, it's all per spec, so totally optional)

inexorabletash · 2018-05-15T21:35:59Z

common/subset-tests.js

+      if (match) {
+        subTestStart = match[1];
+        if (match[2] !== "last") {
+            subTestEnd = match[2];


inexorabletash · 2018-05-15T21:38:34Z

common/subset-tests.js

@@ -0,0 +1,27 @@
+// Only test a subset of tests with, e.g., ?1-10 in the URL


Maybe add that this works well with the <meta name="variant" ... > mechanism ?

inexorabletash · 2018-05-15T21:40:23Z

encoding/legacy-mb-tchinese/big5/big5-encode-form-errors-extBa.html

 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="big5_index.js"></script>
 <script src="big5-encoder.js"></script>
+<script src="/common/subset-tests.js"></script>


Just as a style thing, I'd expect /common/ scripts to be listed under /resources/ and before the module- or test-specific scripts.

Utility code is usually in common I think. I can move the <script> though.

Yeah, sorry, I just meant the script tag

inexorabletash · 2018-05-15T21:48:12Z

common/subset-tests.js

@@ -0,0 +1,27 @@
+// Only test a subset of tests with, e.g., ?1-10 in the URL
+// Sample usage:
+// let currentSubTest = 0;


Hmmm... so every test is going to end up with this sample usage. What do you think about altering (or adding) the API to take care of this, i.e. just exposing a function (e.g. "subsetTest") that takes a closure, which handles the counter/test for the author?

I think this is more flexible and more straightforward to apply to existing tests, since it can vary a lot how tests are being set up. But maybe we can support both somehow...

jimsch · 2018-05-15T22:30:05Z

If this is going to become a common function rather than restricted to a single set of code, I worry about the fact that others are going to want to be able to do both multiple types of variants and combined variants together so that you say one of and one of That means you might need to have a more complicated parsing function

zcorpan · 2018-05-16T04:48:54Z

I made the parsing function support & so it can be combined with something else in the query string.

jimsch · 2018-05-16T05:05:34Z

Looks like it must be the first one however. Should the match be

/^(?|&)(\d+)-(\d+|last)(?:&|$)/

zcorpan · 2018-05-16T06:45:00Z

OK I have addressed the comments, PTAL

annevk · 2018-05-16T06:57:35Z

Should this land together with the Web Crypto changes? Definitely need a different commit message in that case.

It'd be good to document common/subset-tests.js in the documentation. So that when people read about variant (or more likely, forget how it works) they also learn about this.

zcorpan · 2018-05-16T10:59:33Z

Should this land together with the Web Crypto changes? Definitely need a different commit message in that case.

That is now #11028

documentation

I'll look into that now.

Fixes #11015.

zcorpan · 2018-05-24T15:38:31Z

I've overhauled almost all encoding tests. This is ready for review. I might have made some copy-paste mistake somewhere.

annevk · 2018-05-24T15:55:39Z

Could you explain the methodology behind these changes?

zcorpan · 2018-05-24T16:23:52Z

I noticed that there was a lot of common code so I moved it to separate js files and then modified it with the things that needed to vary between tests until I got the same passrate as on master.

annevk · 2018-05-25T07:15:39Z

Thanks, that sounds pretty great. Can you put the non-test resources in a resources/ directory? The other thing I hope we can do at some point is remove the need for normalizeStr, but that'll be easier with this change.

Does this not accidentally revert the removal of normalizeStr I did so far?

zcorpan · 2018-05-25T07:23:44Z

It probably does, will fix

zcorpan · 2018-05-29T12:10:55Z

@annevk PTAL

annevk

Thanks for doing this. I haven't verified before and after for all of these, but I believe you have, and you covered all the gotchas I could think of.

These tests still make me a little angsty given how much code is involved, but this makes it a lot better.

zcorpan · 2018-05-29T13:31:31Z

Hmm, http://web-platform.test:8000/encoding/legacy-mb-japanese/iso-2022-jp/iso2022jp-encode-form-csiso2022jp.html has this difference in test names:

master	zcorpan/encoding-big5-slow
U+A5 ¥ %1B%28%4A%5C	U+A5 ¥ %1B%28%4A%5C%1B%28%42

Is that ok?

annevk · 2018-05-30T07:24:36Z

That seems fine, but do you know why it happens?

zcorpan · 2018-05-31T12:41:50Z

master has normalizeStr(item.expected) instead of just item.expected for the test name for that test. Do you have a preference? We could leave out expected from the test name altogether.

annevk · 2018-05-31T13:07:31Z

Ah, this was removed because of return result.replace(/%1B%28%42$/, "");. I guess I'm fine with your result then.

zcorpan · 2018-05-31T14:00:24Z

The chrome and firefox builds failed, but i suppose that's because it's many slow tests.

The tests have been split in the upstream and should no longer time out: web-platform-tests/wpt#11016 web-platform-tests/wpt#11292 Bug: 736056 Change-Id: I2267ffb05e8c74c5d70bd6d8a4020bb24c78bd0c Reviewed-on: https://chromium-review.googlesource.com/1081123 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Commit-Queue: Robert Ma <robertma@chromium.org> Cr-Commit-Position: refs/heads/master@{#565995}

wpt-pr-bot added assets encoding WebCryptoAPI wg-webcrypto labels May 15, 2018

wpt-pr-bot requested review from annevk, deniak, dontcallmedom, engelke, gsnedders, inexorabletash, jimsch, wafflespeanut and zqzhang May 15, 2018 21:28

inexorabletash reviewed May 15, 2018

View reviewed changes

zcorpan force-pushed the zcorpan/encoding-big5-slow branch from 5e452bd to 6af573f Compare May 16, 2018 10:49

zcorpan mentioned this pull request May 16, 2018

Generalize subsetTest() in common/ and use for WebCryptoAPI tests #11028

Merged

zcorpan force-pushed the zcorpan/encoding-big5-slow branch from 6af573f to cc994a7 Compare May 16, 2018 10:58

zcorpan added the flaky label May 16, 2018

zcorpan added 2 commits May 24, 2018 17:20

Split up big5-encode-form-errors-extB{a,b}.html with variant

169f8fb

Fixes #11015.

Refactor and split up with variant all encoding tests

0e57a45

zcorpan force-pushed the zcorpan/encoding-big5-slow branch from f461d73 to 0e57a45 Compare May 24, 2018 15:28

zcorpan mentioned this pull request May 24, 2018

Possibly missing Hangul tests for euc-kr #11145

Open

zcorpan changed the title ~~Split up big5-encode-form-errors-extB{a,b}.html with variant~~ Refactor and split up with variant all encoding tests May 24, 2018

zcorpan added 2 commits May 29, 2018 13:22

Put js files in resources/

ed1715c

No-op normalizeStr for some tests

40e9f0d

annevk approved these changes May 29, 2018

View reviewed changes

zcorpan added 4 commits May 29, 2018 15:03

Call out that normalizeStr is being overwritten

13b4c01

Preserve desc in test name

880a5c3

Preserve in test names

7e73fd3

Fix mistakes

c67c58f

zcorpan removed wg-webcrypto WebCryptoAPI assets labels May 29, 2018

zcorpan mentioned this pull request May 31, 2018

Fix tests with harness status ERROR or TIMEOUT #11269

Closed

zcorpan merged commit d6c29be into master May 31, 2018

zcorpan deleted the zcorpan/encoding-big5-slow branch May 31, 2018 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and split up with `variant` all encoding tests #11016

Refactor and split up with `variant` all encoding tests #11016

zcorpan commented May 15, 2018 •

edited by wpt-pr-bot

zcorpan commented May 15, 2018

inexorabletash May 15, 2018

inexorabletash May 15, 2018

inexorabletash May 15, 2018

inexorabletash May 15, 2018

inexorabletash May 15, 2018

zcorpan May 16, 2018

inexorabletash May 16, 2018

inexorabletash May 15, 2018

zcorpan May 16, 2018

jimsch commented May 15, 2018

zcorpan commented May 16, 2018

jimsch commented May 16, 2018

zcorpan commented May 16, 2018

annevk commented May 16, 2018

zcorpan commented May 16, 2018

zcorpan commented May 24, 2018

annevk commented May 24, 2018

zcorpan commented May 24, 2018

annevk commented May 25, 2018

zcorpan commented May 25, 2018

zcorpan commented May 29, 2018

annevk left a comment

zcorpan commented May 29, 2018

annevk commented May 30, 2018

zcorpan commented May 31, 2018

annevk commented May 31, 2018

zcorpan commented May 31, 2018

		@@ -0,0 +1,27 @@
		// Only test a subset of tests with, e.g., ?1-10 in the URL

Refactor and split up with variant all encoding tests #11016

Refactor and split up with variant all encoding tests #11016

Conversation

zcorpan commented May 15, 2018 • edited by wpt-pr-bot

zcorpan commented May 15, 2018

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

Choose a reason for hiding this comment

jimsch commented May 15, 2018

zcorpan commented May 16, 2018

jimsch commented May 16, 2018

zcorpan commented May 16, 2018

annevk commented May 16, 2018

zcorpan commented May 16, 2018

zcorpan commented May 24, 2018

annevk commented May 24, 2018

zcorpan commented May 24, 2018

annevk commented May 25, 2018

zcorpan commented May 25, 2018

zcorpan commented May 29, 2018

annevk left a comment

Choose a reason for hiding this comment

zcorpan commented May 29, 2018

annevk commented May 30, 2018

zcorpan commented May 31, 2018

annevk commented May 31, 2018

zcorpan commented May 31, 2018

Refactor and split up with `variant` all encoding tests #11016

Refactor and split up with `variant` all encoding tests #11016

zcorpan commented May 15, 2018 •

edited by wpt-pr-bot