On 07/08/2013 11:56 AM, Mike Connor wrote:
> The thrust of the proposal is simple: we should replace the current
> assortment of keywords, flags, and whiteboard annotations with a
> single multi-state flag (testing-status), and make setting this flag
> part of the required workflow when resolving a bug in a product
> component as FIXED. Developers will be asked to assess whether a bug
> is either: a) not testable, b) covered sufficiently by automated tests
> or c) requires QA to assess the bug. QA then will determine the
> amount and type of testing required, and take action as appropriate.
> It is intended that this formalization of the process will provide us
> visibility into our testing coverage and resource investment across
> our products.
My perspective as a developer who mainly works within js/src and who has
an allergy to using the bugzilla web UI for standard bug workflow:
First, I would prefer that this flag be set when (or just before)
requesting review. The main benefit I see in this process is reminding
the developer to explicitly consider what sort of testing is
appropriate. It's a good time to realize that an automated test should
be written, which means I'll go ahead and write one rather than setting
this flag to some suboptimal value. If the flag is set when landing, or
worse, when the bug is resolved, then it's too late -- I already have
r+, I'm mentally done with the bug unless it crash lands or something,
there's no way I'm going to go back at that point and write a test.
Especially if I'm the sort who feels the need to request review on
tests, and I usually am.
Second, when scanning through the last several patches I have landed, I
find myself wanting another status value: no-test-needed. It is
different from not-testable, because I could imagine a convoluted test
scenario that would test my change, but it would be a waste of my time
to write it and a waste of everyone else's time to run it over and over
as part of an automated suite. This is for things like refactoring or
mostly-refactoring patches, minor fixes, debug-only or depending on
nonstandard config flags or otherwise NPOTB patches, etc. When I scanned
through my commit history, I found a lot of these. I'd probably want
this as the default answer for most of the simple stuff, since
not-testable is often a lie but that doesn't mean it's worth writing a
test for.
Third, there's no way I'd use this if it required me going to the web
UI. I don't do that now for most bugs. In fact, a very common workflow
for me is to write the patch first, then file the bug with bzexport
--new (requesting review with -r at the same time), then reading the
review results in bugmail, then pushing, then using my sekrit |hg
pushed| command to comment with the landed revsets. If you look, you'll
count exactly zero interactions with the bugzilla UI. When attaching a
patch to a bug that somebody else filed, I'll probably go through the UI
once to see what the problem is, then do bzexport and continue through
the workflow above for the rest. (Don't get me wrong; I use the bugzilla
UI all the time for other people's bugs, and when discussion breaks out
on my bugs.)
...which basically means I won't set this flag unless bzexport supports
it. Which is fine, I can implement it there. But at least for myself,
I'd probably default it to no-test-needed (if available, otherwise...
not-testable, maybe? And ignore the lie?) Or maybe even add in some
cleverness so that bzexport would say "if testing-status is unset or
no-test-needed, and there's a test in this patch, set
testing-status=in-automated-testsuite." (The no-test-needed part is
because I'll often put the tests in a separate attachment, and this
logic is applied per-patch.)
And of course, if the flag is set to no-test-needed, then the reviewers
should decide whether they agree. But I'd like to point out that that
isn't straightforward, since a reviewer might click on the 'review' link
in bugmail and only see that one patch, not the flags on the bug. Moar
tooling fixes needed.
That's an excellent way of describing the proposal, btw. Much better
than a wall-o-text post (or wading through a whole thread of them.) In
fact, if I squint, it kinda looks like a bikeshed...
Come to think of it, how about <
http://hotpink.bikeshed.com/> -- just
rename not-testable to no-test-needed (or no-testing-needed)? It doesn't
seem that useful to explicitly identify bugs that cannot be tested. I
mean, it's theoretically interesting to collect lists of bugs for which
an adequate test harness is impractical or ridiculous, or bugs with no
observable effect on functionality (eg pure performance optimizations).
But not worth the developer overhead. Maybe that's what everyone has
been assuming? It looks like Ehsan is, given "...count the absense of
the flag as test-not-needed."
Ehsan's proposal of making unset == no-test-needed makes me a little
nervous, since it conflates no-test-needed with
never-noticed-this-flag-before-is-it-new and
i-havent-thought-about-testability-yet-maybe-ill-get-to-that-later, but
that doesn't seem like a huge deal to me so I'd be fine either way.