Proposal to Stop Naming Files "bugXXXXXX"

[Gregory Szorc]

Following a conversation in #developers, there seems to be a general
consensus that naming files (especially test files) of the form
"bugXXXXXX" is silly and should stop. Therefore, I'm proposing that
reviewers enforce a convention of no *new* files having this naming schema.

Instead, filenames should better reflect what they are actually doing or
involved with. For tests, create filenames that name the entity being
tested. For example, instead of "bug_123456.js," use
"tabbrowser_tab_loading_basic.js."

When adding tests for bugs, add the test case to an existing file which
holds all the tests for that entity. The test case name should describe
what is being tested and the code and commit history can be annotated
with a bug number. e.g.

# Bug 123456 Ensure widget operates as expected
add_test_case(function test_proper_widget_operation() {
...
});

In the end, test cases for related entities are all logically grouped by
filename, not bug sequence. This is much easier for us mere humans.

For reference, I consider the test suite in services/sync/tests/unit [1]
to be of mostly valid form.

There was some talk of establishing a repository hook to enforce this
convention. Personally, I favor social enforcement via code reviews and
auditing to catch accidental offenders. Bug 698906 [2] has been filed to
track whatever we decide is best.

Shall we adopt this proposal?

[1]
https://hg.mozilla.org/mozilla-central/file/6cbeabc07c59/services/sync/tests/unit
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=698906

Gregory Szorc
g...@mozilla.com

[David Mandelin]

On 11/3/2011 10:41 AM, Gregory Szorc wrote:
> Following a conversation in #developers, there seems to be a general
> consensus that naming files (especially test files) of the form
> "bugXXXXXX" is silly and should stop. Therefore, I'm proposing that
> reviewers enforce a convention of no *new* files having this naming schema.
>
> Instead, filenames should better reflect what they are actually doing or
> involved with. For tests, create filenames that name the entity being
> tested. For example, instead of "bug_123456.js," use
> "tabbrowser_tab_loading_basic.js."
>
> When adding tests for bugs, add the test case to an existing file which
> holds all the tests for that entity. The test case name should describe
> what is being tested and the code and commit history can be annotated
> with a bug number. e.g.
>
> # Bug 123456 Ensure widget operates as expected
> add_test_case(function test_proper_widget_operation() {
> ...
> });
>
> In the end, test cases for related entities are all logically grouped by
> filename, not bug sequence. This is much easier for us mere humans.

What about fuzz bugs for JS? It's often not at all obvious what entity
is being tested other than "JS". We could conceivably subclassify them
based on what code had to be changed to fix the bug, but I don't know if
that would be valuable to us. Mostly it's just a random test case, and
other than running it, the most useful thing to be able to do with it is
look up the bug where it was originally fixed in order to read the patch
and discussion.

We also often can't put different tests in one file because the tests
interfere with each other, e.g., by creating properties on the global
object.

Dave

[Jeff Muizelaar]

On 2011-11-03, at 1:41 PM, Gregory Szorc wrote:

> Following a conversation in #developers, there seems to be a general consensus that naming files (especially test files) of the form "bugXXXXXX" is silly and should stop. Therefore, I'm proposing that reviewers enforce a convention of no *new* files having this naming schema.
>
> Instead, filenames should better reflect what they are actually doing or involved with. For tests, create filenames that name the entity being tested. For example, instead of "bug_123456.js," use "tabbrowser_tab_loading_basic.js."
>
> When adding tests for bugs, add the test case to an existing file which holds all the tests for that entity. The test case name should describe what is being tested and the code and commit history can be annotated with a bug number. e.g.
>
> # Bug 123456 Ensure widget operates as expected
> add_test_case(function test_proper_widget_operation() {
> ...
> });
>
> In the end, test cases for related entities are all logically grouped by filename, not bug sequence. This is much easier for us mere humans.
>
> For reference, I consider the test suite in services/sync/tests/unit [1] to be of mostly valid form.
>
> There was some talk of establishing a repository hook to enforce this convention. Personally, I favor social enforcement via code reviews and auditing to catch accidental offenders. Bug 698906 [2] has been filed to track whatever we decide is best.
>
> Shall we adopt this proposal?

Yes. Sometimes a test is just some random stuff that someone ran into and doesn't really fit into any category, in this case I think it's fine to also have the bug number in the test name, but it should be in addition some sort of description.

I don't think there's any need for a repository hook here.

-Jeff

[Bobby Holley]

This makes sense for large, feature-style test coverage (ie,
'test_websocket.html'). But I think that there are very valid reasons
to use the test_bugXXXXXX format.

First, there's the issue of isolation. This is a really great thing:
* It makes it easy to grok the setup and conditions that the test
requires. No need to read through a bunch of unrelated stuff when
you're trying to make things pass.
* It protects tests from each others' side effects.
* It makes it easy to disable individual tests that go orange.
* It protects tests on bugzilla from bitrot.

Furthermore, the naming scheme lets me avoid ruminating on the
platform-wide test architecture when I ponder where to place a new
test. Writing a test is already enough of an effort, and I don't want
to have to dig through all 464 tests in content/base/test to figure
out where my test should go.

-bholley

> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning

[Ehsan Akhgari]

On Thu, Nov 3, 2011 at 1:41 PM, Gregory Szorc <g...@mozilla.com> wrote:

> Following a conversation in #developers, there seems to be a general
> consensus that naming files (especially test files) of the form "bugXXXXXX"
> is silly and should stop. Therefore, I'm proposing that reviewers enforce a
> convention of no *new* files having this naming schema.
>
> Instead, filenames should better reflect what they are actually doing or
> involved with. For tests, create filenames that name the entity being
> tested. For example, instead of "bug_123456.js," use
> "tabbrowser_tab_loading_basic.js."
>

I think your statement is too broad and is incorrect. Naming tests as
bugXXXXXX is not always silly. It totally depends on what the test is
really testing. For example, if I have a test which is supposed to test
the interaction of keyboard event handlings in general, the test can be
named test_keyboard_event_handling or something like that. But if the test
is testing something very specific, such as to make sure that we don't
crash when we flush layout in the middle of an editor update in a document
which has its editable mode switched, naming the test as test_bug123456 is
really the best way I think we can name it, and naming it
test_flush_layout_in_editor_update_if_document_editable_mode_is_switched
would be silly.

I think you can't really generalize a single one-size-fits-all naming
scheme here.

> When adding tests for bugs, add the test case to an existing file which
> holds all the tests for that entity. The test case name should describe
> what is being tested and the code and commit history can be annotated with
> a bug number. e.g.
>
> # Bug 123456 Ensure widget operates as expected
> add_test_case(function test_proper_widget_operation() {
> ...
> });
>

I object to this very strongly. In general, I think we should prefer
smaller, more self-contained tests. Larger tests are harder to understand,
harder to check for correctness, and more likely to suffer from
interdependency issues.

Merging tests just to get the bug numbers out of file names seems like a
very bad idea to me.

> In the end, test cases for related entities are all logically grouped by
> filename, not bug sequence. This is much easier for us mere humans.
>

It actually makes things a lot harder to understand. Try reading this test
for example. ;-) <
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1

>


> For reference, I consider the test suite in services/sync/tests/unit [1]
> to be of mostly valid form.
>
> There was some talk of establishing a repository hook to enforce this
> convention. Personally, I favor social enforcement via code reviews and
> auditing to catch accidental offenders. Bug 698906 [2] has been filed to
> track whatever we decide is best.
>
> Shall we adopt this proposal?
>

No. And until you can make the case why doing this is a net win, we
definitely don't want to ask reviewers to enforce this, or even worse,
enforce it using a repository hook.

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>

[L. David Baron]

The suggestion I've made for reftest file naming in the past, and I
think still support, is the following:

* If you're adding a reftest that's just a simplification of the
original problem in the bug report, it should probably just be
named based on the bug number.

* If you're making an attempt to test additional cases other than
the ones reported in the bug (which is a good thing to do), you
should name cases appropriately, and avoid using the bug number.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla http://www.mozilla.org/ 𝄂

[Ehsan Akhgari]

On Thu, Nov 3, 2011 at 3:03 PM, L. David Baron <dba...@dbaron.org> wrote:

> The suggestion I've made for reftest file naming in the past, and I
> think still support, is the following:
>
> * If you're adding a reftest that's just a simplification of the
> original problem in the bug report, it should probably just be
> named based on the bug number.
>
> * If you're making an attempt to test additional cases other than
> the ones reported in the bug (which is a good thing to do), you
> should name cases appropriately, and avoid using the bug number.

What about the case where the additional tests intend to test other edge
cases related to the bug at hand, which would be very hard to describe in a
short and descriptive file name?

--
Ehsan
<http://ehsanakhgari.org/>

[Steve Fink]

On 11/03/2011 11:38 AM, Ehsan Akhgari wrote:
> On Thu, Nov 3, 2011 at 1:41 PM, Gregory Szorc<g...@mozilla.com> wrote:
>

>> Following a conversation in #developers, there seems to be a general
>> consensus that naming files (especially test files) of the form "bugXXXXXX"
>> is silly and should stop. Therefore, I'm proposing that reviewers enforce a
>> convention of no *new* files having this naming schema.
>>
>> Instead, filenames should better reflect what they are actually doing or
>> involved with. For tests, create filenames that name the entity being
>> tested. For example, instead of "bug_123456.js," use
>> "tabbrowser_tab_loading_basic.js."
>>

> I think your statement is too broad and is incorrect. Naming tests as
> bugXXXXXX is not always silly. It totally depends on what the test is
> really testing. For example, if I have a test which is supposed to test
> the interaction of keyboard event handlings in general, the test can be
> named test_keyboard_event_handling or something like that. But if the test
> is testing something very specific, such as to make sure that we don't
> crash when we flush layout in the middle of an editor update in a document
> which has its editable mode switched, naming the test as test_bug123456 is
> really the best way I think we can name it, and naming it
> test_flush_layout_in_editor_update_if_document_editable_mode_is_switched
> would be silly.
>
> I think you can't really generalize a single one-size-fits-all naming
> scheme here.

I'd definitely vote for always including a bug number, assuming there's
a bug that's even remotely related, *and* always including some
additional text. Even if that additional text doesn't tell you very
much. I just know that even when I'm dealing with a pile of my own
tests, my brain basically treats all numbers as equivalent, so I want
*something* else to distinguish them.

test_bug123456_flush_during_update would be great, but even
test_bug123456_flush would be ok by me (unless the same directory has
test_bug654321_flush and test_bug123654_flush already.) Heck, even
test_bug123456_smurf_jelly would give me half of what I want.

[Clint Talbert]

On 11/3/2011 10:41 AM, Gregory Szorc wrote:

> When adding tests for bugs, add the test case to an existing file which
> holds all the tests for that entity. The test case name should describe
> what is being tested and the code and commit history can be annotated
> with a bug number. e.g.

No, I really disagree with merging tests together. Lots of tests set up
an extended bit of state so that they can then test some sort of
functionality in a highly customized situation. Merging tests like this
will lead to inevitable complexity and errors, and it's going to make it
really, really hard to diagnose and fix intermittent failures.

It's already hard enough to diagnose our intermittent failures. I like
some of the other comments about using the bug number and a description
if possible. I will add too that in the "war on orange" case, it is
often quite useful to go back and revisit first principles about what
this test was originally trying to do so that you can determine why it's
now become intermittent. That's trivial to do when you have the bug
number handy. (And while it's only a few steps more if you have to go
to blame to find it, this is one advantage the bugxxxxx tests have over
their "normally" named counterparts).

Clint

[Dave Townsend]

I think it'd be far better to just make sure every test has a comment at
the top of it explaining the purpose of a test than trying to rely on
conveying that in the filename.

[Honza Bambas]

> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning
>

Big +1. This is what I do with even very simple tests. It helps me (or
anyone) to understand quickly my own test after a long time.

I personally enforce adding well-describing comment to the top of a test
as a review requirement.

I don't understand what is good on merging tests together, and actually
what the exact proposal is. Tests should IMHO be minimalistic as
possible and well separated, unless that would be considered a
performance issue. Also how can I add comments to a bunch of tests in a
single file while preserving good readability?

-hb-

[Shawn Wilsher]

This. Far too often I've reviewed/found test code that did not include
this type of comment, and had to figure it out by looking at the test.

Cheers,

Shawn

[L. David Baron]

Why put it in a comment when you can put it in the test assertion?
For example:

is(s.getPropertyValue("list-style"), "",
"list-style shorthand should be empty when some subproperties specified");

In general, I think things that are self-documenting are better than
things documented out-of-band, because the documentation is more
likely to stay correct over time.

[Dave Townsend]

On 11/6/11 15:14, L. David Baron wrote:
> On Thursday 2011-11-03 21:02 -0700, Dave Townsend wrote:
>> I think it'd be far better to just make sure every test has a
>> comment at the top of it explaining the purpose of a test than
>> trying to rely on conveying that in the filename.
>
> Why put it in a comment when you can put it in the test assertion?
> For example:

I agree that well described assertions are important, but a more general
comment is useful too, especially if the test requires some complex
setup to work

[Dave Townsend]

I think whether to merge or not is very subjective. Some tests for
example may need a lot of code to set up the environment and so it makes
some sense to then do a bunch of tests in that environment rather than
having to just set it up again later (especially if that setup may need
tweaking later).

Generally when I do that I split up the tests into separate functions
and try to put a comment at the top of each function describing what
that part is for.