Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Submission/unipro #135

Merged
merged 25 commits into from Sep 13, 2013

Conversation

Projects
None yet
@sgrekhov
Copy link
Contributor

sgrekhov commented Jun 7, 2013

The first bunch of HTML template tests

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jun 7, 2013

There are a lot of things that need attention.

  • The name of root directory (in this case "htmltemplate") should match the spelling of the specification name that appears in the URL of the specification. So it should be "html-templates" I guess.
  • Ref tests and JS tests should not be in separate directories. They should sit in the same directory, based on the section structure of the specification.
  • Directory names should not contain section numbers; see shadow-dom/ directory for example.
  • Reference file of a ref test should not be in ref/ directory. Instead, you should use *-ref.html format (See WebApps WG's test submission information).
  • Please create separate HTML files for each JS tests. Put test metadata in HTML (using <link> and <meta> elements), rather than JS files. Consult other tests in web-platform-tests for your reference.
  • The link to specification should be stored in "help" metadata. Not "link".
  • Please link to a snapshot of the specification, not the latest one. This helps readers understand what tests need updates.
  • "highlight", "seealso" metadata are not common. Don't use them.
  • In my understanding, "reviewer" field is used to indicate formal review. Don't use it if someone just happened to look at the code. This field is not common in web-platform-tests, so you don't have to use it.
  • Please use descriptive names (in file names, test names, etc.). Don't use cryptic names like A_01_02 or whatever.
  • Please give independent, descriptive names to each test() (or async_test()).
  • Please don't put files that are irrelevant to tests.
  • Please make the commit history clean and understandable. Separate the commit into small and reasonable chunks, and make the commit messages as descriptive as possible. Please don't make a commit just saying "test commit" or something like that.
  • I think HTML5 is preferred over XHTML (but I could not find a reference).

These are all I have found at this moment. Please fix these issues, and I'll take a look again.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Jun 7, 2013

(fwiw "seperate html files per js test" certainly isn't something that is required in general, although I guess if you plan to enforce it for this directory that's your own decision. I would advise against it however; it makes test writing significantly harder and generally discourages people from writing as many tests)

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jun 13, 2013

Main part of the requested changes is done. A copule of questions and comments:

  • RE: "Please create separate HTML files for each JS tests". I'm going to setup nightly build process soon and this build will generate these HTML files.
  • RE: "Please link to a snapshot of the specification, not the latest one". Could you please point at exact version of the specification? Now https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html is used. Additionaly I found only https://dvcs.w3.org/hg/webcomponents/raw-file/c2f82425ba8d/spec/templates/index.html but this is an Editor's draft as well. Which one I should use?
  • RE: ""highlight", "seealso" metadata are not common". I removed seealso but left highlight. It used by our specification coverage tool (not submitted yet) to show what exact part of the specification is covered by the current test
  • RE: "I think HTML5 is preferred over XHTML". Please clarify. Do you mean ref tests?

All other requested changes are done. Please review

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jun 14, 2013

@sgrekhov I'm going to answer each bullet in the order you wrote:

  • OK.
  • http://www.w3.org/TR/2013/WD-html-templates-20130214/ is the latest snapshot at this moment. Files under dvcs.w3.org are the tip-of-tree draft and is subject to change.
  • Did you have a discussion about the coverage tool somewhere? Did you receive some responses? I think the coverage tool is good, but we probably need a consensus on the format and the methodology before doing that.
  • Yes.

I find the commit history is messy and it's hard to review this (having 10 commits just saying "review and formatting" does not sound right); could you consider reordering and cleaning up the history (i.e. dividing the changes into small commits, each of which makes sense as a single independent change)? Also, I'd like the commit messages to be more detailed and descriptive.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jun 14, 2013

I updated Help metadata and changed Ref tests format to HTML5

RE: "Did you have a discussion about the coverage tool?". It wasn't discussed anywhere. Only in emails and people found it convenient and useful. So I'm going to submit it soon and if you found it useless then remove it from the tests (together with "highlight" metadata).

RE: "commit history". Thank you for pointing. We'll change commit messages to be more descriptive

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jun 17, 2013

Yuta-san, please review

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jun 17, 2013

Sorry, didn't have time to look at this. I will make some time tomorrow.

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jun 19, 2013

Sorry for being late. Still I need to talk about the format:

  • The commit history is becoming long. By using commands like git rebase --interactive and git add -p you can restructure the commit history. Structure your commit history so it would consist of independent changes that are logically isolated. Don't think it is just a work log (I know, this is the matter of philosophy, but I can't help saying that).
    • If you do this, git push will fail because the tree is diverging, but you can circumvent with git push --force.
  • Write a script test directly within a HTML file. This seems like a convention everyone follows, and it's convenient for readers.
  • Write the test metadata by using <meta> and <link> tags. I'm not a big fan of the PROPS() hack you are using, and in my opinion, this makes the tests hard to read.
  • Other test suites don't use the section number in directory names, so follow this convention in yours, too.
  • test/ directory doesn't make any sense.
  • Don't put testharness.js etc. into your directory. Instead, refer them as a site-root relative path (e.g. /resources/testharness.js).
  • No need to put test.html that runs all tests.
  • Don't use XML's close tag constructs (e.g. <link ... />). You don't have to close elements like link and meta in HTML5.
  • Use consistent indentation. I don't intend to force a particular style, but stay consistent within the test suite. testharness.js uses 4-space indent, so you might want to do the same.
  • Use consistent naming convention. Don't mix functionName, FUNCTION, FunctionName and function_name. Using different convention is okay if there's a compelling reason, such as "using assert_foobar naming convention to align with testharness.js".
  • Don't put debugging code.

Please refer the relevant documents about the test format, and also look at other test suites in web-platform-tests. Some test suites don't follow the format strictly, but that wouldn't be a reason for you to not follow the rule. There are some documentation you must read:

Note that the second is HTML WG's document and the third is CSS WG's. They are documents from other WGs, but still useful.

Also, keep in mind that other people will read, update or maintain your tests. Authoring tests is not the end of the story. It's actually the beginning.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jun 19, 2013

Yuta-san, thank you for review. Before doing the requested changes I'd like you to take a look to the spec_coverage tool that I just submitted (please run https://github.com/sgrekhov/web-platform-tests/blob/submission/unipro/html-templates/spec/spec_coverage.html). It shows how specification is covered by the tests. This tool had a very good reviews in Shadow DOM project. Requested changes will make the tool non-working because it uses our PROPS, section numbers in test names, tests for the tool should be stored in .js files not in the .html ones etc.
Please take a look at the tool and let me know if it makes sense to have this tool in this project? If Yes then may be it makes sense to make html-templates test suite a little bit non-standard but with the spec_coverage tool?

@odinho

This comment has been minimized.

Copy link
Contributor

odinho commented Jun 19, 2013

There should be no need to have your own meta data function for doing test coverage tools. Please just use the APIs we already have. There are already ways to extract the meta data, and you should rather change testharness.js if it doesn't do something than to introduce another special way to do it.

Also, everything in the above comment makes much sense. Indeed looking at how other test suites are done will give hints to how they look their best. :)

@odinho

This comment has been minimized.

Copy link
Contributor

odinho commented Jun 19, 2013

The above review comment I meant, the points put forward to be fixed there. I'm on a phone :)

Also, we've got a testrunner which reads manifest.txt files and can run tests in sequence, and show a summary. Things like that should not be done for one and one testsuite, but at a higher level.

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jun 19, 2013

I'm neutral regarding the adoption of the coverage tool, but I don't think the tool should be committed to web-platform-tests repository. It should probably be in a new separate repository.

I guess starting a discussion at the public-webapps-testsuite mailing list might be a good way to move forward on the tool...

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jun 20, 2013

I removed the coverage tool and changed test structure according to the Yuta's notes. Please review.
Note: not changed commit history yet. It'll be my next step

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jul 1, 2013

Yuta-san, please review the existing test suite (I'm keeping in ming restructurisation of the commit history, sorry for dalay)

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jul 1, 2013

Let me know once the issues in my prior comments are (mostly) resolved. If I made further comments without making sure outstanding issues are resolved, it would be hard to keep track of what issues were raised, what were not, what were fixed, or what weren't fixed.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jul 1, 2013

Please see my notes inline

  • The commit history is becoming long. By using commands like git rebase --interactive and git add -p you can restructure the commit history. Structure your commit history so it would consist of independent changes that are logically isolated. Don't think it is just a work log (I know, this is the matter of philosophy, but I can't help saying that). If you do this, git push will fail because the tree is diverging, but you can circumvent with git push --force.

Not fixed yet.

  • Write a script test directly within a HTML file. This seems like a convention everyone follows, and it's convenient for readers.

Fixed

  • Write the test metadata by using and tags. I'm not a big fan of the PROPS() hack you are using, and in my opinion, this makes the tests hard to read.

Fixed

  • Other test suites don't use the section number in directory names, so follow this convention in yours, too.

Fixed

  • test/ directory doesn't make any sense.

Fixed

  • Don't put testharness.js etc. into your directory. Instead, refer them as a site-root relative path (e.g. /resources/testharness.js).

Fixed

  • No need to put test.html that runs all tests.

Fixed

  • Don't use XML's close tag constructs (e.g. <link ... />). You don't have to close elements like link and meta in HTML5.

Fixed

  • Use consistent indentation. I don't intend to force a particular style, but stay consistent within the test suite. testharness.js uses 4-space indent, so you might want to do the same.

Fixed but I didn't carefully check all of the source files. There can be some lost files with nonconsistent indention. I'll check the files later. This task is in my list

  • Use consistent naming convention. Don't mix functionName, FUNCTION, FunctionName and function_name. Using different convention is okay if there's a compelling reason, such as "using assert_foobar naming convention to align with testharness.js".

We use functionName naming convention as described at http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml. But testharness uses assert_foobar convention.

  • Don't put debugging code.

I didn't find any. Could you please point to an example of debugging code?

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jul 1, 2013

For debugging code: I see some instances in testcommon.js. Also, my comments on "reviewer" field does not seem to be addressed.

Some more generic comments:

  • Avoid using single-letter variable names; try to use a descriptive name wherever it is feasible.
  • The third argument of assert_*() is optional. If it adds some value (such as helping readers to understand the intention of the assertion), it's okay to have, but it's unnecessary otherwise. Messages that just paraphrases the assertion (like assert_equals(div.parentNode, t.content, 'Wrong foster parent element');) are not particularly useful.
  • It looks like you are adding more tests on the way, which looks bad to me. This is the first work for this test suite, and you are in the stage of adjusting the alignment between your views and W3C test standards; I'd be appreciated if you first focus on and start from a very small set of tests that can represent a baseline of the test suite. Also, adding more tests on the way would increase the reviewer's load for no good reason.

Generally the tests are getting better; I think I can comment on each test in the next round of review.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jul 2, 2013

Yuta-san, thank you for review. Let's take a look to the tests in html-templates/definitions folder. I fixed all of the issues for them, so please review them. If they are ok, I'll change the other tests in the same way. In more details changes are the following:

  • reviewer matadata removed
  • code was formatted to use 4-space indent
  • single-letters variables were renamed to the more descriptive ones
  • debugging and obsolete code was removed from testcommon.js

Only two items are still uncovered.

The first one is the commit history. It'll be my next step.

The second item is the third (optional) argument of assert_*(). We always use the third argument and I believe there are at least two good reasons for that:

  • if test fails this message helps to find assertion that failed. Otherwise tester would have to perform additional investigation to figure out what exact assertion fails
  • this message also can help reader to better understand what the particular assertion checks

So we'd like to preserve this policy "Always add descriptive third argument to all assertions and make it unique if possible".

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Jul 3, 2013

Okay, I'll comment in each test when the commit history becomes clean (it's pretty hard to leave inline comments for now).

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Jul 4, 2013

Yuta-san, we removed all of the history and performed the new commit. Only tests for "Definition" chapter of the spec. Please review

@yutak

This comment has been minimized.

Why don't you use assert_not_equals here?

@yutak

This comment has been minimized.

This comment is more or less equivalent to the test description, so I guess you could remove the comment here (and similar comments elsewhere, too).

@yutak

This comment has been minimized.

Do you mean "contains" here?

This comment has been minimized.

Copy link

yutak replied Jul 5, 2013

Um, I seemed to fail to interpret this, but this description could be better anyways. I'd suggest writing a full sentence here (instead of a noun) to make the test assertion clearer. Also, "(no browsing context)" in the end of the description looks a little awkward and unclear.

@yutak

This comment has been minimized.

"using innerHTML" sounds strange, too. The subject of the verb "use" here is unclear.

@yutak

This comment has been minimized.

I think the following is better:

assert_equals(template.content.nodeType, Node.DOCUMENT_FRAGMENT_NODE, ...);
@yutak

This comment has been minimized.

Is this necessary?

This comment has been minimized.

Copy link
Owner

sgrekhov replied Jul 8, 2013

This is another way to check that template content is a DocumentFragment. Let's check it using the both ways

@yutak

This comment has been minimized.

ID and innerText do not seem to be used.

@yutak

This comment has been minimized.

Some files under resources/ directory are not used.

This comment has been minimized.

Copy link
Owner

sgrekhov replied Jul 8, 2013

The files are used in other tests which are not commited yet

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Aug 1, 2013

Rafael, in the Shadow DOM test suite we checked visibility programmatically. By checking element.offsetTop to determine if element visible or not (invisible element always have offsetTop 0). But one of the Google engineers response to ShadowDOM tests was the following:

• No ref tests where they ought to be some (e.g. using things like ".offsetTop > 0" to test whether the element is rendered, but this is rather weak).

That's why here we use ref tests that requires manual verification.

@tobie

This comment has been minimized.

Copy link
Contributor

tobie commented Aug 1, 2013

Reftests can be automated through WebDriver. I'm actually more concerned about it being difficult to identify reftests as such. The CSS WG uses the .xht extension and three incremental digits for reftests (e.g.: my-test-file-001.xht, my-test-file-002.xht, my-test-file-003.xht, etc.), and reference files are suffixed with -ref (e.g.: my-test-file-001-ref.xht). I suggest we adopt that,

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Aug 1, 2013

Reftests renamed to .xht

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Aug 12, 2013

Hi All.
There's still one open question from my end. Is this ok to use custom function assert_(not)null(...) instead of assert(not)_equals(something, null, ...)?

And if there's no more comments from your end may be it makes sense to pull these tests to master branch?

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Aug 15, 2013

I think either way is fine. I feel like functions like assert_null() should really be defined in testharness.js, though.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Aug 15, 2013

assert_true(... != null) replaced by assert_not_null(...)

@tobie

This comment has been minimized.

Copy link
Contributor

tobie commented Aug 15, 2013

I think either way is fine. I feel like functions like assert_null() should really be defined in testharness.js, though.

+1. Care to write a patch?

}

function assert_null(value, description) {
assert_true(value == null, description);

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Aug 15, 2013

Contributor

This should use === if it exists at all. I don't know why this is better than using assert_equals(x, null), though.

This comment has been minimized.

Copy link
@sgrekhov

sgrekhov Aug 16, 2013

Author Contributor

In this case (===) tests that checks 'undefined' will fail (we use this function to check both null and undefined). assert_null_or_undefined() would be better name for this function but it's rather long

This comment has been minimized.

Copy link
@tobie

tobie Aug 16, 2013

Contributor

Being precise and explicit here is important. Either rename to assert_null_or_undefined or use ===.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Aug 16, 2013

Contributor

And in all the cases I looked at, the test should fail if undefined is returned.

@dglazkov

This comment has been minimized.

Copy link
Contributor

dglazkov commented Aug 23, 2013

@Ms2ger, @tobie -- are you guys okay with this pull request now? If so, feel free to pull the trigger 😃

@maxh

This comment has been minimized.

Copy link

maxh commented Sep 5, 2013

Ping! @Ms2ger, @tobie -- is this ready to be merged into master?

@tobie

This comment has been minimized.

Copy link
Contributor

tobie commented Sep 5, 2013

I'm fine with this, would like @yutak's opinion, though, as he's been reviewing those precisely. I haven't.

@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Sep 5, 2013

I don't see the value of the assert_{,not_}{null,undefined} functions.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Sep 6, 2013

The value of these functions is that:

assert_not_null(something, "Unexpected null")
assert_undefined(something, "Unexpected value")

is more readable than

assert_true(something !== null, "Unexpected null")
assert_false(something === null, "Unexpected null")
assert_false(something !== undefined, "Unexpected value")

Also assert_null is safer because it's easy to make a mistake and write assert_true(something == null,..) instead of ===

So I believe it makes sense to use the functions. But if you insist I can remove them

@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Sep 6, 2013

The comparison you make doesn't make sense to me. These assertions would be written simply as

assert_equals(something, null);
assert_not_equals(something, null);

which doesn't seem so bad to me that you'd need to wrap it into a helper.

@sgrekhov

This comment has been minimized.

Copy link
Contributor Author

sgrekhov commented Sep 9, 2013

I removed the functions and replaced them by

assert_equals(something, null);
assert_not_equals(something, null);

@maxh

This comment has been minimized.

Copy link

maxh commented Sep 10, 2013

Great! @yutak can we get your final approval on this pull request?

@yutak

This comment has been minimized.

Copy link
Contributor

yutak commented Sep 12, 2013

I'm fine with the change ;)

@maxh

This comment has been minimized.

Copy link

maxh commented Sep 13, 2013

@Ms2ger, @tobie -- could you guys merge this? Thanks!

tobie added a commit that referenced this pull request Sep 13, 2013

@tobie tobie merged commit 85c627d into web-platform-tests:master Sep 13, 2013

@sideshowbarker

This comment has been minimized.

Copy link

sideshowbarker commented on 381f0eb Nov 21, 2013

This commit doesn't seem to have changed all the instances of assert_(not)null and assert(not)_undefined. I still find instances remaining in the repo; e.g., in html-templates/additions-to-the-steps-to-clone-a-node/template-clone-children.html, in html-templates/parsing-html-templates/clearing-the-stack-back-to-a-given-context/clearing-stack-back-to-a-table-body-context.html, and other files

This comment has been minimized.

Copy link
Owner Author

sgrekhov replied Nov 21, 2013

Really. Sorry. Fixed

@deniak deniak referenced this pull request Mar 26, 2014

Closed

tests on template element #783

AutomatedTester pushed a commit to AutomatedTester/web-platform-tests that referenced this pull request Dec 1, 2017

Evolve and docs. (web-platform-tests#135)
assoc is now deprecated in favor of evolve.

csnardi added a commit to csnardi/web-platform-tests that referenced this pull request Apr 26, 2018

Squashed 'resources/webidl2/' changes from 8a7ff70664..dd57b9f0db
dd57b9f0db chore(package): update dev deps
7f99dea07c chore(package): bump version number to 10.2.1
c357d4cb01 Add bug link and remove unnecessary quote tests
cd1f7a95ba Add a comment about not using for (const ...)
39a957721c Tokenise a solitary '/' correctly
4efd252f5e Use "let" instead of "var" in for statement
f940514f36 Add count() function. Use idiomatic string methods.
affdfbdc2b Optimise tokenisation and whitespace skipping
ba00d5d9ec refactor: small syntax changes (web-platform-tests#137)
d5c4fbfdfc chore(CHANGELOG): regenerate
3d009b634a chore(CHANGELOG): regenerate
23bb9bb290 chore(CHANGELOG): regenerate
6db306af62 chore(package): bump version number to 10.2.0
ac7ef088f3 fix: solve conflicts from typing union types
f422f2a6f5 feat: type on union types (web-platform-tests#135)
bde0553b49 feat: add const-type for idlTypes (web-platform-tests#132)
c03cd7e8f3 feat: add dictionary/typedef-type (web-platform-tests#133)
6eb1e7f4db feat: add argument/return type (web-platform-tests#134)
d2cfdfd901 feat: add type: attribute-type on idlTypes (web-platform-tests#131)
1e29dcb71b Auto acquisition for parser result changes (web-platform-tests#130)
154eabfbb1 chore(package): update mocha, expect, bump version
36932debd7 Let error messages include the current definition name (web-platform-tests#129)
664f63b61e chore(package-lock): regenerate
536157bffa chore(package): bump version number to 10.0.0
d5b88179df chore(pacakge): upgrade expect dep
0c103b356e Maintain writer.js (web-platform-tests#122)
e2d4467ea1 remove typeExtAttrs from docs
0226b76587 remove iterator documentation (web-platform-tests#123)
e36ae6bf70 BREAKING CHANGE: remove deprecated iterator operation (web-platform-tests#121)
8e73c4ff4c use for-of on tests
c56a921d9b docs(README): iterables ildType is always array
3f39cb152e chore(CHANGELOG): regenerate
ae0060f859 chore(package): bump version number to 9.0.0
24669ed245 BREAKING CHANGE: consistent array type for iterable.idlType (web-platform-tests#117)
f89b5803c7 Update package-lock.json
f581ac63c3 Add myself to contributors in package.json
420ac52ac5 Revert "chore: drop Node 6 support (web-platform-tests#102)"
1c031ed86e chore(CHANGELOG): regenerate

git-subtree-dir: resources/webidl2
git-subtree-split: dd57b9f0db1adbb5712f9fdd6a4e38533ff4ba4b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.