[V2] zipper: several small issues in the tests

[cglacet]

I have several things I think are not correct on this exercise.

First, a small detail, I think there is a bug in the example set_value method (it mutates the object). I don't know if there are other (similar) bugs, I'll test that latter.

Secondly, and most importantly, one of the tests forces the student to do something odd. This is a bit long to explain and I'm not sure I can be really clear on the subject, but here it is:

I think this test (generated from this) is conceptually incorrect:

def test_dead_end(self):
        t1, _, _, _ = self.create_trees()
        zipper = Zipper.from_tree(t1)
        self.assertIsNone(zipper.left().left())

The tree t1 is the following:

     1
    / \
   2   4
    \
     3

While there is indeed no node at location .left().left(), this is still a valid location for a cursor to be at. From this position we could insert a node. And that's sounds like the best way to insert a leaf in a tree as otherwise (if this was returning None as expected in the test) you would have to have two distincts leaf insertion methods insert_left and insert_right. I find it more satisfying to have the following working:

z = Zipper()
z = z.insert(1).right().insert(4).up().left().insert(2).left().insert(3)

This requires having the ability to navigate to non-yet-existing leafs (What?). In that case, the zipper simply contains None as the current tree but still has a context attached to it (and therefore, the zipper is not None, it has a context).

I know that the current tests do not include an insert method, but changing this test would allow to add (leaf) insertions in the goals of the exercise (especially because it would help having a clearer test set).

Why? By adding an insert method, we allow tests to rely only on the capabilities of the Zipper class only, which mean we could totally get rid of the from_tree/to_tree logic (together with the set_left/set_right in the current form). I think It would be nice because the tests would look more natural, which would help students understand zippers. Having to read an input graph (nested dictionaries), to then turn it to a zipper, and finally give it back in the form of a graph (same for as the input) doesn't really help understanding the zipper purpose as this may look totally un-efficient (and it is).

What could be interesting to have as first tests is the immutability of the zipper objects, for example (note that I would like to have all tests using this construction for trees instead of the current zipper_test.py:bt method):

def test_immutability_move_insert(self):
    z = Zipper().insert(1).right().insert(4).up().left().insert(2).left().insert(3).left()
    z_2 = z.insert(5)
    z_3 = z.insert(7)
    self.assertIs((z.focus is None), True)
    self.assertEqual(z.up().value(), 3)
    self.assertNotEqual(z_2.value(), z_3.value())

def test_immutability_set_value(self):
    z = Zipper().insert(1).right().insert(4).up().left().insert(2).left().insert(3)
    z_2 = z.set_value(5)
    self.assertEqual(z.value(), 3)
    self.assertEqual(z_2.value(), 5)

Anyway, let me know what you think about this, if you think I'm not drunk and/or crazy I can suggest a new version of the tests.

[cmccandless]

Thanks for all the thought you've put into this!

First, a small detail, I think there is a bug in the example set_value method (it mutates the object). I don't know if there are other (similar) bugs, I'll test that latter.

I wouldn't be surprised. Since the tests don't verify that the object is not mutates after a value change, and a mutating implementation is much simple to create, this is probably the case.

I really like the idea of adding an insert method, because it changes the structure of a zipper object (in the context of the exercise) from a read-only tree to a mutable one, which is far more practical.

Anyway, let me know what you think about this, if you think I'm not drunk and/or crazy I can suggest a new version of the tests.

I (almost) always suggest this when a contributor suggests test changes/fixes here, whether I like the change or not, because 1) all test changes should go through problem-specifications first, and 2) doing so gives maintainers from other tracks a chance to comment or suggest tweaks to the proposed changes. So by all means, please submit a PR over there! 👍

[cglacet]

I (almost) always suggest this when a contributor suggests test changes/fixes here, whether I like the change or not, because 1) all test changes should go through problem-specifications first, and 2) doing so gives maintainers from other tracks a chance to comment or suggest tweaks to the proposed changes. So by all means, please submit a PR over there! 👍

I'll do that then. How do you usually proceed when writing updating a problem specification. Do you first write all tests in a specific language then try to generalise it as much as possible to fit the "problem specification"'s specification? Or do you proceed the other way around and start with the abstract description first?

[cmccandless]

Generally, starting with the abstract is easier. If working exclusively in the abstract becomes confusing (happens with complex tests), I suggest working in a familiar language (since you're here, I'd suggest Python) in parallel with the abstract.

[cglacet]

There is one thing that I can't find about the problem-specifications format, is how to add things like x = max(2,3) and max(2,3)? (In the .json I have from the older version I can see an "operations" list, which contains "operation", but that doesn't allow for such precision).

[cmccandless]

Is x=max(2,3) just an example? What are you actually trying to do?

The best thing would be to just pick a format that seems to match the existing data as closely as you can, and create a PR. Once there is something there, the active maintainers in problem-specifications will chime in to help settle on a universally acceptable format for what you are trying to achieve.

[cglacet]

It's an example yes, what I want is to be able to say:

zipper = Zipper()
zipper.insert(1)

vs.

zipper = Zipper()
zipper = zipper.insert(1)

[cmccandless]

Take a look at https://github.com/exercism/problem-specifications/blob/master/exercises/react/canonical-data.json and see if that clarifies anything for you. If not, just pick a format that makes sense to you. The problem-specifications maintainers will help smooth it out.

[cglacet]

I have submitted a PR for the specification, should I wait for submitting the python version?

[cmccandless]

@cglacet Standard procedure would be to leave this PR as-is (unmerged, open) until the spec PR has been merged. Then, this PR may be updated (or, if very extensive changes are required, replaced) accordingly and merged.

[stale]

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cmccandless]

Added pinned label as this issue is pending on exercism/problem-specifications#1314

[github-actions]

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[BethanyG]

Since we've both gone through making a test generation template for this exercise and several updates from problem-specifications, I think this issue has largely been addressed. If not, please open a fresh issue against the current form of the exercise. Many thanks for the thoughtful discussion.