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

[docs] Add guide "Making a Testing Plan" #19628

Merged

Conversation

@jugglinmike
Copy link
Contributor

jugglinmike commented Oct 10, 2019

The new tutorials for reftests and testharness.js tests should help orient folks to the technical details of writing tests. They both necessarily assume that contributors know precisely what needs to be tested. I felt like this assumption represented a gap in our support for first-time WPT contributors. This guide is an attempt to fill that gap.

In the past, @chrisdavidmills, @ericholscher, @fantasai, @frivoal, @jihyerish, and @lmccart have all been excellent mentors in practices for welcoming new contributors. I'm mentioning them here to let them know that their influence is still felt and also to welcome their feedback if they can spare the time.

Also /cc @foolip, who helped write the outline for this guide.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Oct 14, 2019

(I have no strong preference here, I'm much more interested in feedback from those with much more experience in technical writing aimed at such audiences!)

Copy link
Contributor

foolip left a comment

I had a lot of comments and focused on what I'd like to see changed, but lest I be misunderstood, I think this will be valuable.

One thing I don't see is the "plan" part, shouldn't this be renamed? Or, can something be said about making a testing plan?

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
standard](https://html.spec.whatwg.org/) describes how the
`localStorage.getItem` method works:

> The `getItem`(*key*) method must return the current value associated with the

This comment has been minimized.

Copy link
@foolip

foolip Oct 19, 2019

Contributor

This is not the best example because likely there's no branch in the implementation, at least not at first level beneath Web IDL bindings. A big branch (a lot in both branches) would be good, maybe something in URL parsing that does something very different depending on the scheme, say?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

That's the reason I chose this text. To me, algorithm steps that describe a condition followed by visually nested sub-steps are much easier to identify as branches. Using this branch as an example demonstrates that branching also occurs in much more subtle forms.

> given *key*. If the given *key* does not exist in the list associated with
> the object then this method must return null.
This algorithm exhibits different behavior depending on whether or not an item

This comment has been minimized.

Copy link
@foolip

foolip Oct 19, 2019

Contributor

This is too wordy. The main audience here, or at least the people I'd like to read this, are already software developers and would have at least a passing understanding of code coverage and possibly a very detailed understanding.

When good tests aren't written, I don't think it's because of a lack of understanding of how code works, but not seeing the value of exploring the corner cases is cross-browser tests. Pointing to real world failures and successes based on test quality might help.

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 21, 2019

Contributor

+1; Philip said everything I was going to, but much better :)

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

I've tightened up the explanation a bit. I don't know any examples to cite, but I'd be happy to include them if anyone has a suggestion.

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor

The wording now sgtm. I don't have any input for examples I'm afraid.


There are many behaviors that are difficult to describe in a succinct file
name. That's commonly the case with low-level rendering details of CSS
specifications. Test authors may resort to generic number-based naming schemes

This comment has been minimized.

Copy link
@foolip

foolip Oct 19, 2019

Contributor

At the time it wasn't a last resort but by convention. It may be best to avoid this discussion here, but @frivoal might have advice on when to use this convention for new tests today.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

By "avoid this discussion," do you mean what I've already written to explain non-semantic file names? Or do you mean the additional guidance you've requested from @frivoal?

This comment has been minimized.

Copy link
@foolip

foolip Oct 22, 2019

Contributor

I mean that this section now implicitly discourages numbered file names, and to the best of my understanding it's still a good idea in many cases today. However, if @frivoal doesn't disapprove of the framing here then I could accept it too.

This comment has been minimized.

Copy link
@zcorpan

This comment has been minimized.

Copy link
@foolip

foolip Oct 22, 2019

Contributor

It may amuse someone to know that only a few hours later I ran into a problem with numbered tests I hadn't considered: #8576 (comment)

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
to know for sure is to review their contents. However, this is a much more
manageable set to work with!

### Querying file contents

This comment has been minimized.

Copy link
@foolip

foolip Oct 19, 2019

Contributor

I think this section could be improved by very briefly pointing out how to search (GitHub or grep) and then focusing on what sorts of pattern actually work, maybe as a table with examples if GitHub and grep patterns diverge enough.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Totally! I can't find anything about relevant search operators in the GitHub documentation, so I've limited the table to regular expressions. Are there any others you'd like to see covered? (I'll bet @zcorpan has ideas)

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 22, 2019

Member

I think the list of examples is good, but maybe point out that if the thing you're searching for is a long unique word (like querySelectorAll or menuitem), then a simple substring search usually is sufficient regardless of what kind of thing it is.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Good idea. Mentioning this makes our recommendation for when to use GitHub Search clearer, and it helps motivate the suggestion for regular expressions and grep.

Copy link
Contributor

stephenmcgruer left a comment

Well, I finally got around to reviewing this and found that Philip had already done a better job -_-. Hopefully there's some signal amongst the noise here, sorry :(.

I agree with Philip on the lack of actual information on 'making a plan' btw, this is more like a set of tips to read when deciding to write some tests. Which is by no means a bad thing, but it isn't a "plan" imo.

docs/writing-tests/index.md Outdated Show resolved Hide resolved
docs/writing-tests/index.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
> given *key*. If the given *key* does not exist in the list associated with
> the object then this method must return null.
This algorithm exhibits different behavior depending on whether or not an item

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 21, 2019

Contributor

+1; Philip said everything I was going to, but much better :)

The most common example may be input validation. Many algorithms for JavaScript
APIs begin by verifying that the input meets some criteria. This may involve
many independent checks. The precise order of the checks may not influence
result of the overall algorithm, but the order is well-defined and observable,

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 21, 2019

Contributor

I think you're correct Philip, but I'd like to note that in web-animations we found many cases where Chrome violated the side-effect expectations of the specs; both in terms of step ordering and stuff like accessing a member when the spec says you can't. It seems trivial, but these things are observable from JS (so much is...) and it's feasible a library could be written that depends on things like accessors being called in the expected order.

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved

Generally speaking, such exhaustive approaches are unlikely to catch more bugs
than a handful of carefully-chosen test cases. Although the risks of dynamic
test generation may be tolerable in some specific cases, it's usually best to

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 21, 2019

Contributor

Despite my above dislike of generated tests, I agree with Philip. Dynamically generating i = 1 to 10,000 is bad, but the sort of dynamic generation seen in web-animations is covering significantly more than 'a handful of carefully-chosen test cases' imo.

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
@birtles

This comment has been minimized.

Copy link
Contributor

birtles commented Oct 21, 2019

I am quite skeptical of generated tests. The web-animations test suite is amazing, but it uses a lot of generated tests and they cause two problems:

i. Hard to read quickly (see https://github.com/web-platform-tests/wpt/blob/master/web-animations/animation-model/animation-types/property-types.js or things like https://github.com/web-platform-tests/wpt/blob/master/web-animations/interfaces/KeyframeEffect/getKeyframes.html),

ii. Maybe worse, really hard to debug quickly. Often pulling out a single failing tests means first finding the underlying 'data-like' file, then figuring out how to comment out the parts you don't need, etc etc.

I think generated tests have value, but are a dangerous tool and can easily stray into being used for their own sake.

Totally agree on both counts (although I don't think animations uses a lot of generated tests--I think they're the exception to the rule). What you've done with the interpolation tests is exactly what I had in mind to do.

@birtles

This comment has been minimized.

Copy link
Contributor

birtles commented Oct 21, 2019

Totally agree on both counts (although I don't think animations uses a lot of generated tests--I think they're the exception to the rule). What you've done with the interpolation tests is exactly what I had in mind to do.

And I think the even bigger danger is that they give the illusion of good test coverage. You can think "we have 1000s of tests and 100% code coverage" but fail to test important corner cases and combinations. (We had that issue with our SMIL implementation where a significant bug went undetected because we dynamically generated 1000s of tests and assumed that meant we had covered the feature well. A few hand-written well-targeted tests would have been much better.)

@stephenmcgruer

This comment has been minimized.

Copy link
Contributor

stephenmcgruer commented Oct 21, 2019

although I don't think animations uses a lot of generated tests--I think they're the exception to the rule

Hah, yes, this is likely true. I have spent a lot of time in those two test areas, so I think they got burned into my brain as what the entire suite for web-animations looks like. I happily defer to the author of most of the suite :).

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Oct 21, 2019

Perhaps for generated tests the recommendation should be that test data for a single test data is a single entry in a JSON list (itself in a JSON file). That way it's somewhat easy to run a single test, add new tests, and run the tests in different environments. I agree that if there's a lot of indirection in how tests are setup it makes them hard to understand.

Copy link
Contributor Author

jugglinmike left a comment

Thanks for the review!

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
standard](https://html.spec.whatwg.org/) describes how the
`localStorage.getItem` method works:

> The `getItem`(*key*) method must return the current value associated with the

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

That's the reason I chose this text. To me, algorithm steps that describe a condition followed by visually nested sub-steps are much easier to identify as branches. Using this branch as an example demonstrates that branching also occurs in much more subtle forms.

> given *key*. If the given *key* does not exist in the list associated with
> the object then this method must return null.
This algorithm exhibits different behavior depending on whether or not an item

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

I've tightened up the explanation a bit. I don't know any examples to cite, but I'd be happy to include them if anyone has a suggestion.

to know for sure is to review their contents. However, this is a much more
manageable set to work with!

### Querying file contents

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Totally! I can't find anything about relevant search operators in the GitHub documentation, so I've limited the table to regular expressions. Are there any others you'd like to see covered? (I'll bet @zcorpan has ideas)

docs/writing-tests/index.md Outdated Show resolved Hide resolved
docs/writing-tests/index.md Outdated Show resolved Hide resolved
@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 22, 2019

One thing I don't see is the "plan" part, shouldn't this be renamed? Or, can
something be said about making a testing plan?

I agree with Philip on the lack of actual information on 'making a plan' btw,
this is more like a set of tips to read when deciding to write some tests.
Which is by no means a bad thing, but it isn't a "plan" imo.

My idea of a testing plan is a summary of the tests that I intend to write.
Does that match what you have in mind? Would it help to add to the
introduction: "You can use these techniques to form a list of the tests you
intend to write."

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 22, 2019

My idea of a testing plan is a summary of the tests that I intend to write.
Does that match what you have in mind? Would it help to add to the
introduction: "You can use these techniques to form a list of the tests you
intend to write."

My expectation from the title would be that the testing plan is an artifact (document) that this guide will help you create. You'd then put that in your spec repo, in wpt, or many in your implementation design doc. These aren't explicitly testing plans, but the sorts of thing I mean:

I don't think that producing the artifact is hugely important, but it would be pretty nice to have for many test suites that have their own conventions informed by the API under test.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 22, 2019

I think I'd suggest going through the review to produce the guidance that we think is most useful, and then see if we think "making a testing plan" is a good title for it. No need to make the content match the title right now.

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved

There are many behaviors that are difficult to describe in a succinct file
name. That's commonly the case with low-level rendering details of CSS
specifications. Test authors may resort to generic number-based naming schemes

This comment has been minimized.

Copy link
@zcorpan
JavaScript string literals ``x = "foo";`` ``(["'])foo\1``
HTML tag names ``<foo attr>`` ``<foo(\s|>|$)``
HTML attributes ``<div foo=3>`` ``<[^>]+\sfoo(\s|>|=|$)``
CSS property name ``style="foo: 4"`` ``([;=\"']|\s|^)foo\s+:``

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 22, 2019

Member

If you want to search for property names in any CSS, you probably want to include matches in CSS files and in <style>, where the preceding character can be {

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Ah, nice!

to know for sure is to review their contents. However, this is a much more
manageable set to work with!

### Querying file contents

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 22, 2019

Member

I think the list of examples is good, but maybe point out that if the thing you're searching for is a long unique word (like querySelectorAll or menuitem), then a simple substring search usually is sufficient regardless of what kind of thing it is.

Copy link
Contributor Author

jugglinmike left a comment

@foolip Previously, this guide listed potential sources of input for JavaScript-based web platform features only. I've replaced that with a table which also gives suggestions about HTML features and CSS features. What do you think?

docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
docs/writing-tests/making-a-testing-plan.md Outdated Show resolved Hide resolved
to know for sure is to review their contents. However, this is a much more
manageable set to work with!

### Querying file contents

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Good idea. Mentioning this makes our recommendation for when to use GitHub Search clearer, and it helps motivate the suggestion for regular expressions and grep.

JavaScript string literals ``x = "foo";`` ``(["'])foo\1``
HTML tag names ``<foo attr>`` ``<foo(\s|>|$)``
HTML attributes ``<div foo=3>`` ``<[^>]+\sfoo(\s|>|=|$)``
CSS property name ``style="foo: 4"`` ``([;=\"']|\s|^)foo\s+:``

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 22, 2019

Author Contributor

Ah, nice!

readers have to mentally "unwind" the iteration to determine what is actually
being verified. The practice is more susceptible to bugs. These bugs may not be
obvious--they may not cause failures, and they may exercise fewer cases than
intended. Finally, tests authored using this approach often have execution time

This comment has been minimized.

Copy link
@foolip

foolip Oct 23, 2019

Contributor

I agree with @zcorpan in 10ae795#r337517178:

I think it's easier to understand the problem by being more direct: When generating tests, it's easy to end up with a lot of tests which introduces the problem that running the tests takes too much time.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 23, 2019

Author Contributor

Trying this:

Finally, tests authored using this approach often take a relatively long time to complete, and that puts a burden on people who collect test results in large numbers.

JavaScript string literals ``x = "foo";`` ``(["'])foo\1``
HTML tag names ``<foo attr>`` ``<foo(\s|>|$)``
HTML attributes ``<div foo=3>`` ``<[^>]+\sfoo(\s|>|=|$)``
CSS property name ``style="foo: 4"`` ``({[;=\"']|\s|^)foo\s+:``

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 23, 2019

Member
Suggested change
CSS property name ``style="foo: 4"`` ``({[;=\"']|\s|^)foo\s+:``
CSS property name ``style="foo: 4"`` ``(style\s*=\s*[\"']?|[{;]|\s|^)foo\s*:``
JavaScript identifier references ``obj.foo()`` ``\bfoo\b``
JavaScript string literals ``x = "foo";`` ``(["'])foo\1``
HTML tag names ``<foo attr>`` ``<foo(\s|>|$)``
HTML attributes ``<div foo=3>`` ``<[^>]+\sfoo(\s|>|=|$)``

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 23, 2019

Member
Suggested change
HTML attributes ``<div foo=3>`` ``<[^>]+\sfoo(\s|>|=|$)``
HTML attributes ``<div foo=3>`` ``<[a-zA-Z][^>]*\sfoo(\s|>|=|$)``

(This still has some false positives like <img sizes='(min-width:500px) 500px, 100vw foo bar' srcset='x 100w, y 200w' src=x alt> but probably good enough for most attribute names.)

@stephenmcgruer stephenmcgruer self-requested a review Oct 23, 2019
@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 23, 2019

@foolip @zcorpan I've switched the example to the "drag" event algorithm, and I've extended the explanation to highlight each of the patterns in general terms (that is: input validation, property access, and event firing). This section feels much stronger for those changes!

Copy link
Contributor

stephenmcgruer left a comment

From my side, this is close to approval. I still have concerns about one section ('Avoid excessive breadth'), and as we discussed offline I would like to see the guide renamed since it doesn't actually cover making a testing plan, but otherwise my comments are just editing really.


Algorithms may accept input from many sources. Modifying the input is the most
direct way we can influence the browser's behavior and verify that it matches
the specifications. That's why it's helpful to be able to recognize all the

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
the specifications. That's why it's helpful to be able to recognize all the
the specifications. That's why it's helpful to be able to recognize

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor

(I felt 'all the different sources of input' was a bit strong)

A thorough test suite for this constructor will include tests for the behavior
of many different values of the *title* parameter and the *options* parameter.
Choosing those values is a challenge unto itself--see [Avoid Excessive

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
Choosing those values is a challenge unto itself--see [Avoid Excessive
Choosing those values is a challenge unto itself - see [Avoid Excessive

The '--' seems to render weirdly in the markdown; should this just be a single hyphen?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 24, 2019

Author Contributor

Philip also noticed this, but since the build we're targeting (the Python Sphinx tool) automatically creates em dashes for this pattern, we're keeping it.

A thorough test suite for this constructor will include tests for the behavior
of many different values of the *title* parameter and the *options* parameter.
Choosing those values is a challenge unto itself--see [Avoid Excessive
Breadth](#avoid-excessive-breadth) for advice on the topic.

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
Breadth](#avoid-excessive-breadth) for advice on the topic.
Breadth](#avoid-excessive-breadth) for advice.
A thorough test suite for this constructor will include tests for the behavior
of many different values of the *title* parameter and the *options* parameter.
Choosing those values is a challenge unto itself--see [Avoid Excessive

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
Choosing those values is a challenge unto itself--see [Avoid Excessive
Choosing those values can be a challenge unto itself--see [Avoid Excessive
the preceding algorithm, it is strictly optional. The test we write for this
should be designated accordingly.

It's important to read these sections carefully, though, because the

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
It's important to read these sections carefully, though, because the
It's important to read these sections carefully, because the
should be designated accordingly.

It's important to read these sections carefully, though, because the
distinction between "mandatory" behavior and "optional" behavior can be quite

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
distinction between "mandatory" behavior and "optional" behavior can be quite
distinction between "mandatory" behavior and "optional" behavior can be

### Don't dive too deep

Algorithms are usually composed of many other algorithms which themselves are

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor
Suggested change
Algorithms are usually composed of many other algorithms which themselves are
Algorithms are often composed of many other algorithms which themselves are
for selector parsing exist and where they are located. That's why it's best to
confer with the people who are maintaining the tests.

### Avoid excessive breadth

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 24, 2019

Contributor

So, I'm not sure where we are on this section atm, but I think currently it reads too negatively towards dynamically generated tests. Yes, this is ironic since I previously argued that dynamic generation of tests is bad, but I think there's a balance and this text strays too far towards 'dont do it'

I would like to see it balanced slightly by acknowledging that this is a complex area, and sometimes it helps and sometimes it doesn't. I would be ok with pushing people away from it, given that this is a guide, but I just don't want to indoctrinate people into thinking never do it.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 24, 2019

Author Contributor

I admit to being more strongly opposed to the practice than most, but I also think that by introducing a consideration without offering some advise, we may make contributors feel more doubtful than empowered. It's sort of like telling someone, "the hardest part about deep-sea fishing is making sure the robotic sharks don't destroy your boat," and moving on to discuss the intricacies of lure selection.

Really, I think we're already risking that. All but one sentence is couched in some way ("can", "often", "may"). I'm surprised that the latest revision feels too forceful. The word "sometimes" already appears in the conclusion:

"Although the risks of dynamic test generation may be tolerable in some specific cases, it’s sometimes best to select the most interesting edge cases and move on."

The risk of indoctrination from a statement like that seems pretty low.

This comment has been minimized.

Copy link
@foolip

foolip Oct 25, 2019

Contributor

The "may be tolerable in some specific cases" wording, which still remains, conveys quite strongly that this usually a very bad idea and that you'd better default to not writing for loops in your tests.

Since I now have @stephenmcgruer on my side of the argument, I'm happy to push back on this again. This, I think, is just fine, and better than unrolling it if the test body is longer:

for (const tagName of ['audio', 'video']) {
  for (const volume of [0, 0.5, 1]) {
    test(() => {
      const element = document.createElement(tagName);
      element.volume = volume;
      assert_something_interesting();
    }, `${tagName} volume ${volume}`);
  }
}

This comment has been minimized.

Copy link
@foolip

foolip Oct 25, 2019

Contributor

Also, maybe advice around this makes more sense somewhere else? This guide is mostly about deciding what and how much to test, not whether to dynamically generate the tests or not.

Copy link
Contributor Author

jugglinmike left a comment

I would like to see the guide renamed since it doesn't actually cover making a testing plan

I wrote this to help people create a list of the tests that they intend to write. To me, that list is a testing plan. @foolip responded to my interpretation in terms of entire specifications and test suites. He referenced documents that seem like guides themselves. I think a guide that helped people make those would be titled "Making a Testing Guide," but I may be misunderstanding.

I'd this document to fit in the vocabulary of the people who use it, but understanding this disconnect simply in terms of terminology may amount to a missed opportunity. It'd be a shame to simply rename the guide if it can be extended it to meet your expectations.

That said, suggestions for alternate titles would help me understand your perspective. So would ideas about substantive changes that would make the title feel more accurate. For example,

  • The latest version might seem too focused on special topics to be considered a complete guide. We could include sections on more straightforward topics so that it's easier to imagine its use from start to finish. Sections titled "Algorithm output" and "Side effects" come to mind
  • The latest version might seem too focused on the patterns. We could explicitly recommend readers keep a list of tests, and we could reference that list from each section (reminding folks to "add to your plan," or "don't plan on testing," as the case may be)
@zcorpan

This comment has been minimized.

Copy link
Member

zcorpan commented Oct 25, 2019

Enumerating a list of cases to write tests for is my idea for what a testing plan is. Example:

#18549 (comment)

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 25, 2019

I also think that #18549 (comment) would qualify as a testing plan, or even my two checkboxes in #6980.

Maybe it's overly pedantic to note that this guide doesn't focus on actually producing the concrete artifact, but it if we think the artifacts are valuable, can we mention them and point to a good example?

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 25, 2019

I don't think there is a generic statement which is both neutral enough to satisfy everyone here and concrete enough to actually help a reader move forward. We've been trying to avoid getting in to detail, but it's starting to feel like that's necessary to produce meaningful advice on the topic.

At the risk of exasperating my reviewers, I've rewritten that paragraph. I've tried to use concrete terms that acknowledge the nuance without derailing into philosophy. It's brief, but I hope it's closer to what we think will empower readers to make good choices.

different assertions within many nested loops. Conversely, the severity would
be low in a test which only iterated over a list of values in order to make the
same assertions about each. Recognizing when the benefits outweigh the risks
requires discretion, so once you understand then, you should use your best

This comment has been minimized.

Copy link
@foolip

foolip Oct 25, 2019

Contributor

Understand then, typo?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 29, 2019

Author Contributor

Good catch. Fixed!

Copy link
Contributor

stephenmcgruer left a comment

At the risk of exasperating my reviewers, I've rewritten that paragraph. I've tried to use concrete terms that acknowledge the nuance without derailing into philosophy. It's brief, but I hope it's closer to what we think will empower readers to make good choices.

I like the new version!

In general, this LGTM now. Philip has some good comments about the 'testing plan' discussion, sounds to me like y'all will work out something sensible without further input from me :)

> node](https://html.spec.whatwg.org/multipage/dnd.html#source-node).
> 3. [...]
A strong test suite will verify that the `drop` event is fired as specified,

This comment has been minimized.

Copy link
@stephenmcgruer
and `dragend`
events](https://bugs.chromium.org/p/chromium/issues/detail?id=1005747), and as
a result, real web applications stopped functioning. If there had been a test
fpr the sequence of these events, then this confusion would have been avoided.

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Oct 25, 2019

Contributor
Suggested change
fpr the sequence of these events, then this confusion would have been avoided.
for the sequence of these events, then this confusion would have been avoided.

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 29, 2019

Author Contributor

How silly. Fixed

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 30, 2019

Member

From GitHub's changed files view, this doesn't look fixed to me. Is it fixed?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Oct 30, 2019

Author Contributor

I forgot to push. Rookie mistake, sorry about that.

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 29, 2019

I also think that #18549 (comment) would qualify as a testing plan, or even my two checkboxes in
#6980.

Maybe it's overly pedantic to note that this guide doesn't focus on actually producing the concrete artifact, but it if we think the artifacts are valuable, can we mention them and point to a good example?

I've included those examples and added a third (to cover the opposite extreme of the spectrum on level-of-detail). I've also refocused the main content to be in terms of the "plan," and deferred recommendations about actually authoring tests to the very end of the guide.

This made me realize that there was an important issue about browser state which we hadn't mentioned yet--untestable features. The guide (and this review) is already pretty lengthy, but I feel this is important enough to justify adding a few more sentences (especially considering the concept is not present anywhere else in the documentation).

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 30, 2019

I won't have time to give this another round of review this week, but am happy to see this merged given that @stephenmcgruer had similar concerns to me and has now approved this. I'll give it another read over in its published form and file any issues I spot as follow up.

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 30, 2019

One more change: now that gh-17894 has landed, we can recommend the testharness.js tutorial to readers as they finish reading this guide. We're already recommending the reftest tutorial, primarily because as a more narrative and introductory document, it's a more logical "next step" for this audience. Recommending the testharness.js tutorial as well (instead of the API documentation) is more consistent and more fluid.

> node](https://html.spec.whatwg.org/multipage/dnd.html#source-node).
> 3. [...]
A strong test suite will verify that the `drop` event is fired as specified,

This comment has been minimized.

Copy link
@zcorpan

zcorpan Oct 30, 2019

Member

This is not addressed as far as I can tell.

Copy link
Contributor

stephenmcgruer left a comment

Still LGTM

@jugglinmike jugglinmike merged commit fa36b9e into web-platform-tests:master Oct 30, 2019
10 checks passed
10 checks passed
build-and-publish
Details
Azure Pipelines Build #20191030.67 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - safari[experimental] Safari results
Details
@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 30, 2019

The guide is now published online!

https://web-platform-tests.org/writing-tests/making-a-testing-plan.html

Thanks to everyone who helped make this something worth reading :)

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.