This is an archive of the discontinued Mercurial Phabricator instance.

tests: consistently put #testcases at beginning of file
AbandonedPublic

Authored by martinvonz on Mar 13 2020, 1:34 AM.

Details

Reviewers
durin42
marmoute
mharbison72
Group Reviewers
hg-reviewers
Summary

I think it's misleading to put #testcases statements further down in
the file because any statements before it will be executed in all
cases. I also find them easier to find at the beginning of the file.

This patch moves the #testcases to the top of the files, which
matches where we have the #requires. I'll leave it to others to move
high-level test file descriptions to before both of these kinds of
statements if they care about that.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Mar 13 2020, 1:34 AM

The other change looks good and I would take them if they were not in the same changeset as the change in tests/test-push-race.t

tests/test-push-race.t
1–3

Please avoir putting content before the test file title.
(and maybe keep the documentation closer to them.)

marmoute requested changes to this revision.Mar 13 2020, 4:51 AM
This revision now requires changes to proceed.Mar 13 2020, 4:51 AM
martinvonz added inline comments.Mar 13 2020, 9:29 AM
tests/test-push-race.t
1–3

Please avoir putting content before the
test file title.

Why? (I personally don't find this hard to read.)

mharbison72 accepted this revision.Mar 13 2020, 9:52 AM
mharbison72 added a subscriber: mharbison72.
mharbison72 added inline comments.
tests/test-push-race.t
1–3

I'd go further and say it's easier to read when #testcases is the very first line, as it doesn't get visually swallowed by nesting between two long lines. (I didn't realize they didn't have to be the first line, and wouldn't have looked anywhere else either.)

I would rather have the large title (===\ntitle\n===) be the very first things in the test for clarify.

I very much agree with Matt

I would rather have the large title (===\ntitle\n===) be the very first things in the test for clarify.

I'm like @mharbison72 and mostly skip past the #testcases and #requires at the top (like imports and includes in other languages), so they don't bother me at all. Sounds like we have two votes in favor and one against. Let's see what others feel.

(like imports and includes in other languages)

Precisely, import and includes usually comes after the initial module licence, title and documentation. So I would like the testcase declaration to comes after the initial module title (and maybe doc).

(like imports and includes in other languages)

Precisely, import and includes usually comes after the initial module licence, title and documentation. So I would like the testcase declaration to comes after the initial module title (and maybe doc).

That's fair. I assume we should do the same with #require in that case? That'll be quite a bit larger, I think (because I think we usually put them before the test description).

That's a good point, doing it for #require too would be more consistent.

I would make a difference between test that do not make too much of an effort to have a title + early documentation and the one who actually make effort to have a formal format for their title and documentation (the one with =====\ntitle\n===== or title\n=====).

I would be fine with having the requires/testcases right after the title when it make senses. I think there are some instance where the variants are formally documented, and the #testcase block is part of that. It might make sense to keep them at that location.

That's a good point, doing it for #require too would be more consistent.
I would make a difference between test that do not make too much of an effort to have a title + early documentation and the one who actually make effort to have a formal format for their title and documentation (the one with =====\ntitle\n===== or title\n=====).
I would be fine with having the requires/testcases right after the title when it make senses. I think there are some instance where the variants are formally documented, and the #testcase block is part of that. It might make sense to keep them at that location.

I'll leave this patch as is and leave the moving of comments above #testcase and #requires for you to do in a follow-up (if you care enough about it; I still prefer having the all these # statements at the top).

Can you drop your change to tests/test-push-race.t from this patch ?

Can you drop your change to tests/test-push-race.t from this patch ?

That change is consistent with the other changes in putting #requires and #testcases first. I'll update the commit message.

martinvonz edited the summary of this revision. (Show Details)Mar 13 2020, 12:57 PM

Can you drop your change to tests/test-push-race.t from this patch ?

That change is consistent with the other changes in putting #requires and #testcases first. I'll update the commit message.

This change break the formatting of an otherwhise well formatted file (tests/test-narrow-sparse.t). Can you please drop it from this diff and move forward with the others?

martinvonz abandoned this revision.Mar 13 2020, 1:11 PM