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

Jump to bottom

Docs: JavaScript tests #24903

Merged
merged 9 commits into from Sep 3, 2020
Merged

Docs: JavaScript tests #24903

merged 9 commits into from Sep 3, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented Aug 6, 2020 •
edited

This tries to pull some of the more pertinent content forward.

TODO:

  • create more links into testharness-api
  • figure out how to weave in testdriver.js references
  • rewrite idlharness to account for all the changes to how we do that today

Feedback appreciated.

@annevk annevk requested a review from jgraham August 6, 2020 13:58
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 6, 2020 14:01 Inactive
@annevk
Docs: JavaScript tests
7e9c6ce 
This tries to pull some of the more pertinent content forward.

TODO:

* create more links into testharness-api * figure out how to weave in testdriver.js references
* rewrite idlharness to account for all the changes to how we do that today

Feedback appreciated.
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 26, 2020 10:45 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 26, 2020 11:44 Inactive
@annevk annevk marked this pull request as ready for review August 26, 2020 12:24
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 26, 2020 12:25 Inactive
jgraham
jgraham reviewed Aug 26, 2020
View reviewed changes
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the testharness.js docs changes lgtm; I didn't read the idlharness ones as closely since others use that more than me,

docs/writing-tests/testharness.md

Use `// META: timeout=long` at the beginning of the resource.

### Specifying test [variants](#variants) in auto-generated boilerplate tests
### Specifying test [variants](#variants)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little odd to mention variants here before they're introduced below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I make it part of the variants section somehow? It definitely seems better to have variants listed later on.

docs/writing-tests/testharness.md Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented Aug 26, 2020

Thanks, I hope @stephenmcgruer can review the IDL part.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 28, 2020 13:56 Inactive
jugglinmike
jugglinmike approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Hexcles
Hexcles approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I like the change overall and just have some nitpicky/optional suggestions.

docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness.md
This page describes testharness.js exhaustively; [the tutorial on writing a
testharness.js test](testharness-tutorial) provides a concise guide to writing
a test--a good place to start for newcomers to the project.
## Window tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how this might be a more logical structure for test authors! As a tooling/infra maintainer, I always think of it as "I write the test page" vs. "server generates the test page" on the top level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, server documentation is still suboptimal and might warrant another look to ensure common patterns are covered adequately.

sideshowbarker
sideshowbarker approved these changes Aug 29, 2020
View reviewed changes
docs/writing-tests/testharness.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness.md Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 31, 2020 10:26 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 31, 2020 10:34 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24903 August 31, 2020 10:43 Inactive
@annevk annevk closed this Aug 31, 2020
@annevk annevk reopened this Aug 31, 2020
@annevk
Copy link
Member Author

annevk commented Aug 31, 2020

I plan on merging this tomorrow. Let me know if you need more time.

stephenmcgruer
stephenmcgruer approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDL section LGTM with two non-blocking suggestions

docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
docs/writing-tests/idlharness.md Outdated Show resolved Hide resolved
@annevk annevk mentioned this pull request Sep 2, 2020
Merged
@annevk
Copy link
Member Author

annevk commented Sep 2, 2020

Thanks @stephenmcgruer for highlighting that. I created #25327, #25331, #25334, #25353, #25354, #25355, and #25356, as well as some PRs that are already merged, to convince myself that we have no need for those methods and everything we have can be done with idl_test plus the remaining two methods on IdlArray. (There's still some consumers left beyond those PRs but they can all be converted as far as I can tell and I might well do those too.)

I'll give this another day before merging given the changes I just made to tighten up the idlharness documentation.

@annevk
Copy link
Member Author

annevk commented Sep 2, 2020

The one instance I forgot about is idlArray.add_untested_idls('typedef Window WindowProxy;'); in HTML. But let's consider that quirk in IDL itself for not knowing about the Window/WindowProxy interaction.

stephenmcgruer
stephenmcgruer approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM - thanks for going the extra mile here!

@annevk annevk merged commit e867131 into master Sep 3, 2020
@annevk annevk deleted the annevk/docs-js-tests branch September 3, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenmcgruer stephenmcgruer stephenmcgruer approved these changes

@sideshowbarker sideshowbarker sideshowbarker approved these changes

@jugglinmike jugglinmike jugglinmike approved these changes

@Hexcles Hexcles Hexcles approved these changes

@jgraham jgraham Awaiting requested review from jgraham

Assignees

@sideshowbarker sideshowbarker

Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@annevk @stephenmcgruer @sideshowbarker @jgraham @jugglinmike @Hexcles @wpt-pr-bot @moz-wptsync-bot