You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/03/26 20:01:09 UTC
[PATCH] Don't use XFAIL for unwritten tests; also rm redundant test
runs
"XFail" is intended to mean that a test is failing because of a known bug or
deficiency in Subversion. I noticed that some tests were using it as a way of
inserting a place-holder for a test which might one day be written. This patch
removes those place-holders.
There's one possible ill effect: the test index numbers of some existing tests
are changed by this patch. I think that's acceptable; certainly we have at
times inserted new tests into the sequence, and tests/cmdline/README doesn't
say we should avoid changing test numbers. If anyone thinks this is a problem
then we'd better invent an "unused test" marker; I considered using "Skip" but
that's not right.
This patch also removes the redundant running of six tests in schedule_tests.py
that are also called within later tests in that same file. Although it can be
handy to have a different test number for each possible type of failure, having
the test suite run in a reasonably short time is important and outweighs the
benefit in this case.
- Julian
Re: [PATCH] Don't use XFAIL for unwritten tests; also rm redundant test runs
Posted by "S.Ramaswamy" <sr...@gmail.com>.
On 3/27/06, Julian Foad <ju...@btopenworld.com> wrote:
> "XFail" is intended to mean that a test is failing because of a known bug or
> deficiency in Subversion. I noticed that some tests were using it as a way of
> inserting a place-holder for a test which might one day be written. This patch
> removes those place-holders.
>
> There's one possible ill effect: the test index numbers of some existing tests
> are changed by this patch. I think that's acceptable; certainly we have at
> times inserted new tests into the sequence, and tests/cmdline/README doesn't
> say we should avoid changing test numbers. If anyone thinks this is a problem
> then we'd better invent an "unused test" marker; I considered using "Skip" but
> that's not right.
>
> This patch also removes the redundant running of six tests in schedule_tests.py
> that are also called within later tests in that same file. Although it can be
> handy to have a different test number for each possible type of failure, having
> the test suite run in a reasonably short time is important and outweighs the
> benefit in this case.
>
> - Julian
>
>
> Remove "XFail" where it was used inappropriately for tests that have not been
> written, and remove some redundant re-testing.
>
> * subversion/tests/cmdline/schedule_tests.py
> Rewrite the comment describing the first set of tests, to reflect this
> reorganisation.
> (commit_add_files, commit_add_directories, commit_nested_adds,
> commit_add_executable, commit_delete_files, commit_delete_dirs):
> Delete these unwritten test stubs.
> (test_list): Remove the six Stage I tests because they are duplicated in
> Stage II, and the six unwritten Stage III tests (which were marked XFail).
>
> * subversion/tests/cmdline/trans_tests.py
> (enable_translation, checkout_translation, disable_translation): Comment out
> these unwritten test stubs.
> (test_list): Comment out those three tests (which were marked XFail).
>
> Index: subversion/tests/cmdline/schedule_tests.py
> ===================================================================
> --- subversion/tests/cmdline/schedule_tests.py (revision 19040)
> +++ subversion/tests/cmdline/schedule_tests.py (working copy)
> @@ -35,19 +35,17 @@
> #
> # Each test must return on success or raise on failure.
> #
> -# NOTE: Tests in this section should be written in triplets. First
> -# compose a test which make schedule changes and local mods, and
> -# verifies that status output is as expected. Secondly, compose a
> -# test which calls the first test (to do all the dirty work), then
> -# test reversion of those changes. Finally, compose a third test
> -# which, again, calls the first test to "set the stage", and then
> -# commit those changes.
> -#
> -#----------------------------------------------------------------------
>
> #######################################################################
> # Stage I - Schedules and modifications, verified with `svn status'
> #
> +# These tests make schedule changes and local mods, and verify that status
> +# output is as expected. In a second stage, reversion of these changes is
> +# tested. Potentially, a third stage could test committing these same
> +# changes.
> +#
> +# NOTE: these tests are run within the Stage II tests, not on their own.
> +#
>
> def add_files(sbox):
> "schedule: add some files"
> @@ -249,6 +247,9 @@
> #######################################################################
> # Stage II - Reversion of changes made in Stage I
> #
> +# Each test in Stage II calls the corresponding Stage I test
> +# and then also tests reversion of those changes.
> +#
>
> def check_reversion(files, output):
> expected_output = []
> @@ -383,63 +384,11 @@
> '--recursive', wc_dir)
> check_reversion(files, output)
>
> +
> #######################################################################
> -# Stage III - Commit of modifications made in Stage 1
> +# Regression tests
> #
>
> -def commit_add_files(sbox):
> - "commit: add some files"
> -
> - add_files(sbox)
> -
> - raise svntest.Failure
> -
> -#----------------------------------------------------------------------
> -
> -def commit_add_directories(sbox):
> - "commit: add some directories"
> -
> - add_directories(sbox)
> -
> - raise svntest.Failure
> -
> -#----------------------------------------------------------------------
> -
> -def commit_nested_adds(sbox):
> - "commit: add some nested files and directories"
> -
> - nested_adds(sbox)
> -
> - raise svntest.Failure
> -
> -#----------------------------------------------------------------------
> -
> -def commit_add_executable(sbox):
> - "commit: add some executable files"
> -
> - add_executable(sbox)
> -
> - raise svntest.Failure
> -
> -
> -#----------------------------------------------------------------------
> -
> -def commit_delete_files(sbox):
> - "commit: delete some files"
> -
> - delete_files(sbox)
> -
> - raise svntest.Failure
> -
> -#----------------------------------------------------------------------
> -
> -def commit_delete_dirs(sbox):
> - "commit: delete some directories"
> -
> - delete_dirs(sbox)
> -
> - raise svntest.Failure
> -
> #----------------------------------------------------------------------
> # Regression test for issue #863:
> #
> @@ -685,24 +634,12 @@
>
> # list all tests here, starting with None:
> test_list = [ None,
> - add_files,
> - add_directories,
> - nested_adds,
> - Skip(add_executable, (os.name != 'posix')),
> - delete_files,
> - delete_dirs,
> revert_add_files,
> revert_add_directories,
> revert_nested_adds,
> Skip(revert_add_executable, (os.name != 'posix')),
> revert_delete_files,
> revert_delete_dirs,
> - XFail(commit_add_files),
> - XFail(commit_add_directories),
> - XFail(commit_nested_adds),
> - Skip(XFail(commit_add_executable), (os.name != 'posix')),
> - XFail(commit_delete_files),
> - XFail(commit_delete_dirs),
> unschedule_missing_added,
> delete_missing,
> revert_inside_newly_added_dir,
> Index: subversion/tests/cmdline/trans_tests.py
> ===================================================================
> --- subversion/tests/cmdline/trans_tests.py (revision 19040)
> +++ subversion/tests/cmdline/trans_tests.py (working copy)
> @@ -349,33 +349,31 @@
> print "Id expansion failed for", id_with_space_path
> raise svntest.Failure
> fp.close()
> -
> -def enable_translation(sbox):
> - "enable translation, check status, commit"
>
> - raise svntest.Failure
> +#----------------------------------------------------------------------
> +
> +#def enable_translation(sbox):
> +# "enable translation, check status, commit"
> +
> # TODO: Turn on newline conversion and/or keyword substitution for all
> # sorts of files, with and without local mods, and verify that
> # status shows the right stuff. The, commit those mods.
>
> #----------------------------------------------------------------------
>
> -def checkout_translated():
> - "checkout files that have translation enabled"
> +#def checkout_translated():
> +# "checkout files that have translation enabled"
>
> - raise svntest.Failure
> # TODO: Checkout a tree which contains files with translation
> # enabled.
>
> #----------------------------------------------------------------------
>
> -def disable_translation():
> - "disable translation, check status, commit"
> +#def disable_translation():
> +# "disable translation, check status, commit"
>
> - raise svntest.Failure
> # TODO: Disable translation on files which have had it enabled,
> # with and without local mods, check status, and commit.
> -
>
> #----------------------------------------------------------------------
>
> @@ -757,9 +755,9 @@
> # list all tests here, starting with None:
> test_list = [ None,
> keywords_from_birth,
> - XFail(enable_translation),
> - XFail(checkout_translated),
> - XFail(disable_translation),
> + # enable_translation,
> + # checkout_translated,
> + # disable_translation,
> update_modified_with_translation,
> eol_change_is_text_mod,
> keyword_expanded_on_checkout,
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Don't use XFAIL for unwritten tests; also rm redundant
test runs
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Collins-Sussman wrote:
> On 3/26/06, Julian Foad <ju...@btopenworld.com> wrote:
>
>>"XFail" is intended to mean that a test is failing because of a known bug or
>>deficiency in Subversion. I noticed that some tests were using it as a way of
>>inserting a place-holder for a test which might one day be written. This patch
>>removes those place-holders.
>>
>>There's one possible ill effect: the test index numbers of some existing tests
>>are changed by this patch.
>
> Why not just Skip() the test-stubs, rather than outright delete them?
> Wouldn't that be a lot less disruptive?
I don't think that having some of the test numbers change can cause a
significant problem. It's not like we ever don't know which version of the
test suite is being run.
If we think it is a problem, we ought to make a conscious decision and document
that we Don't Do That. (While I generally try to avoid changing the numbers of
existing tests, I believe I have, within the last few months, approved at least
one patch that inserted a new test between existing tests, and seen more than one.)
The output "SKIP: schedule_tests.py 1: schedule: add some files" implies to me
that the test was skipped because it was inappropriate on this platform for
some reason, not that the named test is in fact just an idea for a test that
doesn't actually exist. I think if we wanted a way of listing a place-holder
for a test that doesn't actually exist, we should invent something specifically
for the purpose rather than using "Skip".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Don't use XFAIL for unwritten tests; also rm redundant
test runs
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> Tests that aren't implemented should either be deleted, or succeed. I
> think the latter is the worse choice.
Thanks. Committed in r19132.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Don't use XFAIL for unwritten tests; also rm redundant
test runs
Posted by Branko Čibej <br...@xbc.nu>.
Ben Collins-Sussman wrote:
> On 3/26/06, Julian Foad <ju...@btopenworld.com> wrote:
>
>> "XFail" is intended to mean that a test is failing because of a known bug or
>> deficiency in Subversion. I noticed that some tests were using it as a way of
>> inserting a place-holder for a test which might one day be written. This patch
>> removes those place-holders.
>>
>> There's one possible ill effect: the test index numbers of some existing tests
>> are changed by this patch.
>>
>
> Why not just Skip() the test-stubs, rather than outright delete them?
> Wouldn't that be a lot less disruptive?
>
Changing test numbers are not a problem, especially as we have test
descriptions, too.
Tests that aren't implemented should either be deleted, or succeed. I
think the latter is the worse choice.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Don't use XFAIL for unwritten tests; also rm redundant test runs
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On 3/26/06, Julian Foad <ju...@btopenworld.com> wrote:
> "XFail" is intended to mean that a test is failing because of a known bug or
> deficiency in Subversion. I noticed that some tests were using it as a way of
> inserting a place-holder for a test which might one day be written. This patch
> removes those place-holders.
>
> There's one possible ill effect: the test index numbers of some existing tests
> are changed by this patch.
Why not just Skip() the test-stubs, rather than outright delete them?
Wouldn't that be a lot less disruptive?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org