Patchwork D8282: tests: consistently put #testcases at beginning of file

login
register
mail settings
Submitter phabricator
Date March 13, 2020, 5:34 a.m.
Message ID <differential-rev-PHID-DREV-gjc557zk6zacno6kvpjc-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45765/
State Superseded
Headers show

Comments

phabricator - March 13, 2020, 5:34 a.m.
martinvonz created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

AFFECTED FILES
  tests/test-fix-topology.t
  tests/test-narrow-sparse.t
  tests/test-push-race.t

CHANGE DETAILS




To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2020, 8:51 a.m.
marmoute added a comment.


  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

INLINE COMMENTS

> test-push-race.t:1-3
> +#testcases strict unrelated
> +
>  ============================================================================================

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - March 13, 2020, 1:29 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> marmoute wrote in test-push-race.t:1-3
> Please avoir putting content before the test file title.
> (and maybe keep the documentation closer to them.)

> Please avoir putting content before the
> test file title.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 13, 2020, 1:52 p.m.
mharbison72 added inline comments.
mharbison72 accepted this revision.

INLINE COMMENTS

> martinvonz wrote in 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.)

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.)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 2:15 p.m.
marmoute added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 3:05 p.m.
martinvonz added a comment.


  I very much agree with Matt
  
  In D8282#123660 <https://phab.mercurial-scm.org/D8282#123660>, @marmoute wrote:
  
  > 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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 3:29 p.m.
marmoute added a comment.


  In D8282#123661 <https://phab.mercurial-scm.org/D8282#123661>, @martinvonz wrote:
  
  > (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).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 3:34 p.m.
martinvonz added a comment.


  In D8282#123663 <https://phab.mercurial-scm.org/D8282#123663>, @marmoute wrote:
  
  > In D8282#123661 <https://phab.mercurial-scm.org/D8282#123661>, @martinvonz wrote:
  >
  >> (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).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 3:38 p.m.
marmoute added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 4:35 p.m.
martinvonz added a comment.


  In D8282#123665 <https://phab.mercurial-scm.org/D8282#123665>, @marmoute wrote:
  
  > 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).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 4:49 p.m.
marmoute added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 4:54 p.m.
martinvonz added a comment.


  In D8282#123675 <https://phab.mercurial-scm.org/D8282#123675>, @marmoute wrote:
  
  > 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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel
phabricator - March 13, 2020, 5:10 p.m.
marmoute added a comment.


  In D8282#123677 <https://phab.mercurial-scm.org/D8282#123677>, @martinvonz wrote:
  
  > In D8282#123675 <https://phab.mercurial-scm.org/D8282#123675>, @marmoute wrote:
  >
  >> 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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8282/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8282

To: martinvonz, durin42, #hg-reviewers, marmoute, mharbison72
Cc: mharbison72, marmoute, mercurial-devel

Patch

diff --git a/tests/test-push-race.t b/tests/test-push-race.t
--- a/tests/test-push-race.t
+++ b/tests/test-push-race.t
@@ -1,3 +1,5 @@ 
+#testcases strict unrelated
+
 ============================================================================================
 Test cases where there are race condition between two clients pushing to the same repository
 ============================================================================================
@@ -7,6 +9,10 @@ 
 perform its push. The "raced" client starts its actual push after the "racing"
 client push is fully complete.
 
+We tests multiple cases:
+* strict: no race detected,
+* unrelated: race on unrelated heads are allowed.
+
 A set of extension and shell functions ensures this scheduling.
 
   $ cat >> delaypush.py << EOF
@@ -113,12 +119,6 @@ 
   > graph = log -G --rev 'sort(all(), "topo")'
   > EOF
 
-We tests multiple cases:
-* strict: no race detected,
-* unrelated: race on unrelated heads are allowed.
-
-#testcases strict unrelated
-
 #if strict
 
   $ cat >> $HGRCPATH << EOF
diff --git a/tests/test-narrow-sparse.t b/tests/test-narrow-sparse.t
--- a/tests/test-narrow-sparse.t
+++ b/tests/test-narrow-sparse.t
@@ -1,7 +1,8 @@ 
+#testcases tree flat
+
 Testing interaction of sparse and narrow when both are enabled on the client
 side and we do a non-ellipsis clone
 
-#testcases tree flat
   $ . "$TESTDIR/narrow-library.sh"
   $ cat << EOF >> $HGRCPATH
   > [extensions]
diff --git a/tests/test-fix-topology.t b/tests/test-fix-topology.t
--- a/tests/test-fix-topology.t
+++ b/tests/test-fix-topology.t
@@ -1,3 +1,5 @@ 
+#testcases obsstore-off obsstore-on
+
 A script that implements uppercasing all letters in a file.
 
   $ UPPERCASEPY="$TESTTMP/uppercase.py"
@@ -30,7 +32,6 @@ 
 we'll test it with evolution off and on. This only changes the revision
 numbers, if all is well.
 
-#testcases obsstore-off obsstore-on
 #if obsstore-on
   $ cat >> $HGRCPATH <<EOF
   > [experimental]