Tests for Zipper are far from sufficient

[joshgoebel]

Reference: https://exercism.io/mentor/solutions/962ebabf19e848e5bceed0533f1898ad#discussion-post-605355

We should add a test similar to:

test('traversal returns a new Zipper object', () => {
    expect(zipper.value()).toEqual(1);
    let left = zipper.left();
    expect(zipper.value()).toEqual(1);
    expect(left.value()).toEqual(2);
  })

This seems to be a pretty key thing to not test for.

I don't actually think the students exact implementation is super critical here. What they did was implement a single object with a "cursor" that kept track of where it was in the tree and acted accordingly. So that calling left or right would merely update the cursor position...

Which 100% works since the current tests never require that a new object be generated.

[joshgoebel]

Related of course: #777

Thought from my reading/understanding this behavior would still be the same even if we made massive changes to the exercise. So I think this is a separate issue.

[SleeplessByte]

Instead of adding that test, each function should have a test that should assert that the object returned is a new instance, per the description. I think that will solve all the issues.

We'll accept a PR on this repository, but please link back to problem-specifications. I'm okay with the change NOT being accepted there, but us still applying it here.

[joshgoebel]

You mean left and right and up?

Should left/up in combo result in 3 new objects or would up return me to the original?

[SleeplessByte]

From the README.md:

prev (move the focus to the previous child of the same parent, returns a new zipper)
next (move the focus to the next child of the same parent, returns a new zipper)
up (move the focus to the parent, returns a new zipper)
set_value (set the value of the focus node, returns a new zipper)
insert_before (insert a new subtree before the focus node, it becomes the prev of the focus node, returns a new zipper)
insert_after (insert a new subtree after the focus node, it becomes the next of the focus node, returns a new zipper)
delete (removes the focus node and all subtrees, focus moves to the next node if possible otherwise to the prev node if possible, otherwise to the parent node, returns a new zipper)

So we need a test for these methods to check if they're immutable. I like the idea of guiding tests as mentioned in #777 as well.

[joshgoebel]

Those are a sample list, they don't apply to the actual implementation used here. Which is another confusing point, maybe that's mentioned in #777.

[joshgoebel]

And you didn't answer me specifically:

Should left/up in combo result in 3 new objects or would up return me to the original?

Or perhaps you did... are you saying you'd expect 3 new objects then, ie you never return to any prior object?

left/up 100 times results in 200 Zippers pointing to 2 actual nodes?

I ask because in my implementation right now parent returns you to the prior object... so I want to make sure I'm 100% clear on the requirements if I'm considering updating all the tests myself.

[SleeplessByte]

I meant that you could extrapolate from the README. The stub has the following methods that should return a new object:

• left
• right
• up
• setValue

SIDE NOTE: I absolutely dislike that we use left and right instead of prev / next, so we NEED a HINTS file for this exercise. See CONTRIBUTING.md and follow the instructions for generating a README, which will explain how we can add extra information to the README.

Or perhaps you did... are you saying you'd expect 3 new objects then, ie you never return to any prior object?

When one of the four methods is called above, a new object should be returned, per the description. So yes, if we call .left().up() 100 times, you should have generated 200 zippers. At least, that's what the "functional" immutable approach would be. They all have their own "pointers", but they all point to the same nodes. That is correct.

I ask because in my implementation right now parent returns you to the prior object

It doesn't if you look at the CI example. It recreates a new Zipper each up. That does NOT mean that the internal structure isn't re-used. I don't think that's clean, but it's not a requirement, persé.

[joshgoebel]

Oh I forgot we had those sample solutions. :-)

Oh, setting is suppose to make new zippers too? Yeah, even my own solution is doing this way wrong... starting to sound like a uber functional exercise. :)

They all have their own "pointers", but they all point to the same nodes. That is correct.

When I hear pointer I think of the object property, not the object itself... Ie Zipper#focusNode is the pointer, since it's literally pointing at the node... but I see how you'd come to think of the higher level object as doing the same thing, despite it really just holding the pointer.

[joshgoebel]

SIDE NOTE: I absolutely dislike that we use left and right instead of prev / next

I thought that's because we were a tree, where left right makes sense. A Zipper (from my understanding) can be used with any type of data structure. The structure isn't part of the spec per se...

[SleeplessByte]

I thought that's because we were a tree, where left right makes sense. A Zipper (from my understanding) can be used with any type of data structure. The structure isn't part of the spec per se...

Correct, but it's part of the description. If we add a HINTS file we can clarify that in this exercise we'll be working with a Tree that has a left and right branch. That will clear it up, agreed?

Oh, setting is suppose to make new zippers too? Yeah, even my own solution is doing this way wrong... starting to sound like a uber functional exercise. :)

I think that was the intention. It also makes it much more likely that people will make everything immutable from the get-go 💯

[joshgoebel]

Correct, but it's part of the description. If we add a HINTS file we can clarify that in this exercise we'll be working with a Tree that has a left and right branch. That will clear it up, agreed?

Sure, or we could just change the example entirely. Does it really further the goals of Exercism by having one example in the readme and then an entirely different actual case to be solved? I'm not so sure.

[SleeplessByte]

It doesn't. But for now this is all we can do, because I want to keep following the problem-specifications readme format. I don't want to have to keep track "which exercises have a special readme" as often we mass re-generate all the readme's.

In v3 this is fixed! So it's only a concern for now anyway. :)