Page MenuHomePhabricator

Explain better inside arcanist / Differential app what a test plan is
Closed, WontfixPublic

Description

Even after explaining it to the company, we get new people starting to use Phabricator, or we have people who don't use it often and forget, and the result is that we often need to remind people what "Test plan' means and what to put in there (typically people put nothing or write how to test, not how they actually tested the change).

It would be very helpful if there were some inline tooltips in both arcanist and the Differential app with concrete explanations of what to put in title, summary and test plan.

Event Timeline

swisspol raised the priority of this task from to Needs Triage.
swisspol updated the task description. (Show Details)
swisspol added subscribers: swisspol, rfergu.

Here's the documentation on what a good test plan is --

https://secure.phabricator.com/book/phabricator/article/differential_test_plans/

I would be a bit concerned about surfacing this in the arcanist workflow right in the template; the "average" case would end up having to delete a bunch of stuff. For many editors this is a non-trivial additional amount of time - e.g. anything without mouse support. We could perhaps have a link in the footer or something to the above doc.

Differential on the other hand feels like we could have placeholder text in a given field.

Overall though, I am not sure this problem of people not filling out test plans correctly gets significantly better from this sort of solution. Rather, I would expect some cultural / managerial force around the importance of testing would be the solution for any organization with this issue. (i.e. if folks in the organization started thinking about their code the way the test plan document says, the test plan flows naturally...)

I agree that putting a bunch of text that needs to be deleted each time is not the right approach. Maybe as part of the arcanist flow, after saving the diff but before sending it to Phabricator it could prompt (assuming test plans are required in the config):

Did you write a proper test plan?
(one or two sentences go here)
For more information, visit ...
[y/N]

And some sort of instructions on how not to be prompted again.

vaguely related: There's lots of good texts in diviner, but they are kind of hard to find for a new user. Reorganizing them, and maybe a strong separation of "new user stuff" and more advanced stuff might help.

Mostly, I think we should break up "Phabricator User Docs" book to "install and config" and "user" books, and somehow link to the user guide from the front page directly in some obvious way; Then we can send new users to read something they won't be lost in.

chad triaged this task as Low priority.Sep 11 2014, 4:50 PM
chad added a project: Documentation.

@epriestley - two specification type things I could use your help on here. No rush though.

What do you think of comment from @swisspol https://secure.phabricator.com/T6028#5 ?

What do you think of comment from @avivey https://secure.phabricator.com/T6028#6 ? I agree with the notion that the books could be broken out into better higher level books but am not sure about the correct taxonomy. I also wonder if perhaps some articles should be in multiple books?

Things I think we should definitely do:

  • hyperlink "Test Plan" header in the RevisionView controller to link to the documentation
  • put some placeholder text in the "test plan" textarea when creating a diff from the web UI that says "List repeatable steps that thoroughly test your change(s)."
  • possibly hyperlink the "Test plan" label for this textarea like on the "Test Plan" header

...and then maybe another thing or three based on what you think on the questions.

What do you think of comment from @swisspol https://secure.phabricator.com/T6028#5 ?

I don't think we should do this:

  • I don't recall other installs having difficulty with test plans.
  • The error message when you fail to provide a test plan is clear: You must provide a test plan. Describe the actions you performed to verify the behavior of this change.
  • The docs have a detailed explanation of test plans, and they're the first hit on Google for "phabricator test plan".
  • I generally think it's OK to be somewhat vague about test plans and let culture drive them, similar to how we don't mandate exactly what a summary should look like.
  • I generally agree with you about the cultural aspect of this.
  • This potentially interacts badly with T4924 (non-interactive execution of arc diff), although that's a fairly distinct issue.
  • T4631 (allowing the server to issue warnings) will supersede this. After T4631, @swisspol could add a custom field which raises this warning when a user authors their first revision.
  • Generally, everything about arc and arc diff particularly is already too hard, and I think adding more prompts and decisions tends to make it harder. I'd like to reduce the time and complexity between users running arc for the first time and generating a diff they're happy with (T6072 touches on this), even if that means they didn't get all of the fields exactly right.

What do you think of comment from @avivey https://secure.phabricator.com/T6028#6 ?

Maybe. Reservations I have:

  • In general, this makes sense on its face.
  • I think we'll have to update all (some of?) the code to point at the right book, since we assume all links are into the "User Documentation" book right now, I believe. The redirect magic might get this right without changes, though. This potentially breaks somewhat-out-of-date installs a little bit, depending on how magical I made the magic.
  • There are a lot of articles which are don't clearly go in a "Config" vs "Usage" book. For example, the "Arcanist Quick Start Guide" has both installation/configuration and usage instructions. I think once we started dividing them up we might find a lot of cases like this, where a topic is not clearly administrative or user-facing, or two closely related topics end up in different books. I worry this might reduce the accessibility of the documentation.
  • In broad strokes, I worry that documentation is just not that useful, and this isn't really an organization or indexing problem, but a fundamental problem of documentation.
    • Subjectively, I didn't see any change in user behaviors after the last Diviner rework, even though it improved the organization of the documentation substantially and made it much easier to access and read. In particular, it moved all user-relevant topics to a dedicated section. Previously, they were mixed in with a bunch of technical/contributor documentation and all of the class and function documentation. I think there isn't room left to make another organization step of that magnitude, but that huge step had little visible effect.
    • Also subjectively, non-documentation solutions seem universally more effective than documentation solutions. For example, introducing the setup issues was a huge leap forward in the ease of installing Phabricator and every time we add a new issue it pretty much stops all reports of that problem from users, but most of what they tackle was already available in the documentation. I don't think we could ever achieve even a modest fraction of that effect through better organization. (Documenting issues seemingly does relatively little to stop reports in many cases, although it does make responding to those reports easier.)
    • A lot of the value of the documentation is that we can point users to it, and organization isn't very important for this.

Things I think we should definitely do:

  • hyperlink "Test Plan" header in the RevisionView controller to link to the documentation
  • put some placeholder text in the "test plan" textarea when creating a diff from the web UI that says "List repeatable steps that thoroughly test your change(s)."
  • possibly hyperlink the "Test plan" label for this textarea like on the "Test Plan" header

At least personally, I don't think we should bother with any of this: it's inconsistent (we don't do it for other fields), it happens too late in the process (the user has already typed a "bad" test plan), I don't think this is really a widespread issue, I don't think it's a big deal if users get it wrong (if no one is correcting them, is it really that important?) and T4631 will provide a way for installs to force users through a terms-and-conditions check on their test plan (or whatever other fields) if they really want this.

btrahan claimed this task.

Thanks for the thoughts. Sounds to me like T4631 will provide the infrastructure to fix this for folks who still have issues.

On documentation, more specific feedback would be awesome - like "this article here is confusing because..." - but let's file new tasks as appropriate as that comes up.