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.

Download Raw Diff

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

Herald added a reviewer: durin42. · View Herald TranscriptMar 13 2020, 1:34 AM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

Herald added a subscriber: mercurial-devel. · View Herald Transcript

martinvonz added a child revision: D8283: tests: simplify test-fix-topology.t slightly by using a `(case !)`.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

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

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

In D8282#123663, @marmoute wrote:

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

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.

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

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

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

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

In D8282#123677, @martinvonz wrote:

In 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?

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

Revision Contents
Changeset List

			Path	Packages
M			tests/test-fix-topology.t (3 lines)
M			tests/test-narrow-sparse.t (3 lines)
M			tests/test-push-race.t (12 lines)

Commit	Parents	Author	Summary	Date
d4e830a2e46a	47f8c741df0f	Martin von Zweigbergk		Mar 13 2020, 1:23 AM

Status	Author	Revision
Closed	martinvonz	D8288 fix: mark -r as advanced
Closed	martinvonz	D8287 fix: add a -s option to format a revision and its descendants
Closed	martinvonz	D8286 fix: move handling of --all into getrevstofix() for consistency
Closed	martinvonz	D8285 fix: refactor getrevstofix() to define revisions first, then validate them
Closed	martinvonz	D8284 fix: disallow `hg fix --all --working-dir`
Closed	martinvonz	D8283 tests: simplify test-fix-topology.t slightly by using a `(case !)`
Abandoned	martinvonz	D8282 tests: consistently put #testcases at beginning of file

Diff 20768

tests/test-fix-topology.t

				#testcases obsstore-off obsstore-on

	A script that implements uppercasing all letters in a file.			A script that implements uppercasing all letters in a file.

	$ UPPERCASEPY="$TESTTMP/uppercase.py"			$ UPPERCASEPY="$TESTTMP/uppercase.py"
	$ cat > $UPPERCASEPY <<EOF			$ cat > $UPPERCASEPY <<EOF
	> import sys			> import sys
	> from mercurial.utils.procutil import setbinary			> from mercurial.utils.procutil import setbinary
	> setbinary(sys.stdin)			> setbinary(sys.stdin)
	> setbinary(sys.stdout)			> setbinary(sys.stdout)
	> uppercase-whole-file:command="$PYTHON" $UPPERCASEPY			> uppercase-whole-file:command="$PYTHON" $UPPERCASEPY
	> uppercase-whole-file:pattern=set:**			> uppercase-whole-file:pattern=set:**
	> EOF			> EOF

	This tests the only behavior that should really be affected by obsolescence, so			This tests the only behavior that should really be affected by obsolescence, so
	we'll test it with evolution off and on. This only changes the revision			we'll test it with evolution off and on. This only changes the revision
	numbers, if all is well.			numbers, if all is well.

	#testcases obsstore-off obsstore-on
	#if obsstore-on			#if obsstore-on
	$ cat >> $HGRCPATH <<EOF			$ cat >> $HGRCPATH <<EOF
	> [experimental]			> [experimental]
	> evolution.createmarkers=True			> evolution.createmarkers=True
	> evolution.allowunstable=True			> evolution.allowunstable=True
	> EOF			> EOF
	#endif			#endif

tests/test-narrow-sparse.t

				#testcases tree flat

	Testing interaction of sparse and narrow when both are enabled on the client			Testing interaction of sparse and narrow when both are enabled on the client
	side and we do a non-ellipsis clone			side and we do a non-ellipsis clone

	#testcases tree flat
	$ . "$TESTDIR/narrow-library.sh"			$ . "$TESTDIR/narrow-library.sh"
	$ cat << EOF >> $HGRCPATH			$ cat << EOF >> $HGRCPATH
	> [extensions]			> [extensions]
	> sparse =			> sparse =
	> EOF			> EOF

	#if tree			#if tree
	$ cat << EOF >> $HGRCPATH			$ cat << EOF >> $HGRCPATH

tests/test-push-race.t

				#testcases strict unrelated

	============================================================================================			============================================================================================
				marmouteUnsubmitted Not Done Please avoir putting content before the test file title. (and maybe keep the documentation closer to them.) marmoute: Please avoir putting content before the test file title. (and maybe keep the documentation…
				martinvonzAuthorUnsubmitted Done Please avoir putting content before the test file title. Why? (I personally don't find this hard to read.) martinvonz: > Please avoir putting content before the > test file title. Why? (I personally don't find…
				mharbison72Unsubmitted Not Done 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.) mharbison72: I'd go further and say it's easier to read when `#testcases` is the very first line, as it…
	Test cases where there are race condition between two clients pushing to the same repository			Test cases where there are race condition between two clients pushing to the same repository
	============================================================================================			============================================================================================

	This file tests cases where two clients push to a server at the same time. The			This file tests cases where two clients push to a server at the same time. The
	"raced" client is done preparing it push bundle when the "racing" client			"raced" client is done preparing it push bundle when the "racing" client
	perform its push. The "raced" client starts its actual push after the "racing"			perform its push. The "raced" client starts its actual push after the "racing"
	client push is fully complete.			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.			A set of extension and shell functions ensures this scheduling.

	$ cat >> delaypush.py << EOF			$ cat >> delaypush.py << EOF
	> """small extension orchestrate push race			> """small extension orchestrate push race
	>			>
	> Client with the extensions will create a file when ready and get stuck until			> Client with the extensions will create a file when ready and get stuck until
	> a file is created."""			> a file is created."""
	>			>
	> [phases]			> [phases]
	> publish = no			> publish = no
	> [experimental]			> [experimental]
	> evolution=true			> evolution=true
	> [alias]			> [alias]
	> graph = log -G --rev 'sort(all(), "topo")'			> graph = log -G --rev 'sort(all(), "topo")'
	> EOF			> EOF

	We tests multiple cases:
	* strict: no race detected,
	* unrelated: race on unrelated heads are allowed.

	#testcases strict unrelated

	#if strict			#if strict

	$ cat >> $HGRCPATH << EOF			$ cat >> $HGRCPATH << EOF
	> [server]			> [server]
	> concurrent-push-mode = strict			> concurrent-push-mode = strict
	> EOF			> EOF

	#endif			#endif

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20768			Mar 13 2020, 1:34 AM	★	★