On Tue, 2008-08-19 at 01:34 +0200, Neels Janosch Hofmeyr wrote:
> Sorry, a debugging line had crept into that patch!
> Reposting.
Hi Neels. This looks like a good move in general.
Do you think this framework can replace my "tree_conflict_tests.py"? It
would be good to do so, because I invented a new framework which was
useful to me but not compatible with the rest of the test files and a
bit difficult for other people to extend.
I am having difficulty reviewing the tests. There seems to be a lot of
"magic numbers" in a sense. Not numbers, but the expected results. For
example:
[[[
> + leaf_edit = svntest.actions.deep_trees_leaf_edit
> + tree_del = svntest.actions.deep_trees_tree_del
[...]
> + expected_status = svntest.wc.State('', {
> + '' : Item(status=' M', wc_rev='7'),
> + 'D' : Item(status=' ', wc_rev='7'),
> + 'D/D1' : Item(status='D ', wc_rev='7'),
> + 'D/D1/delta' : Item(status='D ', wc_rev='8'),
> + 'F' : Item(status='C ', wc_rev='7'),
> + 'F/alpha' : Item(status='D ', wc_rev='8'),
> + 'DD' : Item(status=' ', wc_rev='7'),
> + 'DD/D1' : Item(status='D ', wc_rev='7'),
> + 'DD/D1/D2' : Item(status='D ', wc_rev='7'),
> + 'DD/D1/D2/epsilon' : Item(status='D ', wc_rev='8'),
> + 'DF' : Item(status=' ', wc_rev='7'),
> + 'DF/D1' : Item(status='D ', wc_rev='7'),
> + 'DF/D1/beta' : Item(status='D ', wc_rev='8'),
> + 'DDD' : Item(status=' ', wc_rev='7'),
> + 'DDD/D1' : Item(status='D ', wc_rev='7'),
> + 'DDD/D1/D2' : Item(status='D ', wc_rev='7'),
> + 'DDD/D1/D2/D3' : Item(status='D ', wc_rev='7'),
> + 'DDD/D1/D2/D3/zeta' : Item(status='D ', wc_rev='8'),
> + 'DDF' : Item(status=' ', wc_rev='7'),
> + 'DDF/D1' : Item(status='D ', wc_rev='7'),
> + 'DDF/D1/D2' : Item(status='D ', wc_rev='7'),
> + 'DDF/D1/D2/gamma' : Item(status='D ', wc_rev='8'),
> + })
> +
> + expected_skip = svntest.wc.State('', {})
> +
> + greater_scheme +=
> [ DeepTreesTestCase("local_leaf_edit_incoming_tree_del",
> + leaf_edit,
> + tree_del,
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip) ]
]]]
This is like a regression test: it says "This is the output from a
previous run, and let me know if anything changes." But when developing
a new feature, it can be more useful to have tests that each check one
specific aspect of the behaviour. For example, 1: in these scenarios,
test that a tree conflict indicator is printed by the "merge" command.
2: Where there is an unresolved tree conflict, test that a commit fails.
3: Test that the "resolve" command works. etc.
When I write a test like that, or when I review one that you have
written, it's quite tedious to work out what all the item paths will be,
or what the status and revision number should be for each path.
(Your other patch to make the test suite print out the status tree in
this format is one way to get the data that results from the current
implementation, but it doesn't help me to decide whether the data are
correct.)
I would like the test to say something like this (pseudo-Python):
[[[
> + leaf_edit = svntest.actions.deep_trees_leaf_edit
> + tree_del = svntest.actions.deep_trees_tree_del
[...]
> + # Set the default expected status of each item
> + expected_status = svntest.actions.deep_trees_items
> + for item in tree_del:
> + expected_status[item] = Item(status='D ', wc_rev='8')
> +
> + # But each item that conflicts should get a 'C' on its parent
> + # and not increment its revision number.
> + for item in leaf_edit:
> + if item is in tree_del:
> + parent = item.get_parent()
> + expected_status[parent] = Item(status='C ', wc_rev='7')
> +
> + greater_scheme +=
> [ DeepTreesTestCase("local_leaf_edit_incoming_tree_del",
> + leaf_edit,
> + tree_del,
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip) ]
]]]
Do you think that would be a better way to write the tests so that they
are understandable?
If you do, I could commit your current patch soon and we could make that
a later improvement.
- Julian
> [[[
> Replacing file-based tree-conflicts cmdline tests. The new tests are
> concerned with directory *and* file victims, in different tree depths.
> Contains new tests for update, switch and merge operations.
>
> * subversion/tests/cmdline/svntest/actions.py
> (set_up_tree_conflicts): Remove old helper for update, switch.
> (set_up_tree_conflicts_for_merge): Remove old helper for merge.
> (make_tc_test_trees): Rename and split new helper into make_deep_trees()
> and add_deep_trees().
> (tc_leaf_del): Rename new helper to deep_trees_leaf_del().
> (tc_tree_del): Rename new helper to deep_trees_tree_del().
> (tc_tree_del): Rename new helper to deep_trees_tree_del().
> (tc_text_append): Rename new helper to deep_trees_leaf_edit().
> (set_up_deep_tree_conflicts_up): Remove function, its purpose replaced by
> deep_trees_run_tests_scheme_for_update().
> (set_up_deep_tree_conflicts_sw): Remove function, its purpose replaced by
> deep_trees_run_tests_scheme_for_switch().
> (add_deep_trees): New function, replacing make_tc_test_trees().
> (make_deep_trees): New function, used by add_deep_trees().
> (deep_trees_virginal_state): New wc.State structure.
> (deep_trees_leaf_edit): New function.
> (deep_trees_after_leaf_edit): New wc.State structure.
> (deep_trees_leaf_del): New function.
> (deep_trees_after_leaf_del): New wc.State structure.
> (deep_trees_tree_del): New function.
> (deep_trees_after_tree_del): New wc.State structure.
> (DeepTreesTestCase): New class.
> (deep_trees_run_tests_scheme_for_update): New function.
> (deep_trees_run_tests_scheme_for_switch): New function.
> (deep_trees_run_tests_scheme_for_merge): New function.
>
> * subversion/tests/cmdline/update_tests.py
> (tree_conflicts_in_updated_files): Remove old test, replaced by
> tree_conflicts_on_update().
> (tree_conflicts_on_update): New test, replacing
> tree_conflicts_in_updated_files(). Adds directory victims, and
> conflicts in different tree depths.
>
> * subversion/tests/cmdline/switch_tests.py
> (tree_conflicts_in_switched_files): Remove old test, replaced by
> tree_conflicts_on_switch().
> (tree_conflicts_on_switch): New test, replacing
> tree_conflicts_in_switched_files(). Adds directory victims, and
> conflicts in different tree depths.
>
> * subversion/tests/cmdline/merge_tests.py
> (tree_conflicts_in_merged_files): Remove old test, replaced by
> tree_conflicts_on_merge().
> (tree_conflicts_on_merge): New test, replacing
> tree_conflicts_in_merged_files(). Adds directory victims, and conflicts
> in different tree depths. Adds another case to the old merge test,
> namely omitting a local commit.
> ]]]
>
> Neels Janosch Hofmeyr wrote:
> > Hi!
> >
> > The current cmdline tests for tree-conflicts (in update_tests.py,
> > switch_tests.py and merge_tests.py) only test for file victims.
> >
> > The new tests in this patch still do exactly what the old tests did (file
> > victims), but they also test for directory victims and (more) file victims
> > in a large set of tree-conflict cases in varying depths of directory trees.
> >
> > AFAIK, this new set is complete in that it covers every imaginable case of
> > tree conflicts, for all of update, switch and merge.
> >
> > I started off with sbutler's new (unfinished) test scheme which is already
> > committed on the tree-conflicts branch, and took it to a workable state. I
> > removed the old file-based tests, after checking for identical results.
> >
> > Currently, most of these new tests still yield undesirable results. The
> > course that I aim for is this:
> >
> > 1) Commit the new tree-conflicts tests, so that they PASS the current
> > tree-conflicts branch, even though many test results are still
> > undesirable. (This patch)
> >
> > 2) One by one, look at the undesirable results and fix them in subsequent
> > patches. Patches should both fix the situation and make the tests
> > pass the fixed situation.
> >
> > So, as mentioned, this patch passes the current situation on the
> > tree-conflicts branch. But, one test case cannot be passed, because it fails
> > in a segmentation fault!
> >
> > I have investigated this and posted a preliminary patch in another mail:
> > http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=141887
> >
> > I would like this patch to be committed on the tree-conflicts branch, so
> > that continuing work on fixing tree-conflicts situations can be comprehensive.
> >
> > Thanks!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-19 17:57:56 CEST