Repository navigation
Tests for Zipper are far from sufficient #838
Description
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.
Activity
joshgoebel commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
From the
README.md: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 commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
And you didn't answer me specifically:
Or perhaps you did... are you saying you'd expect 3 new objects then, ie you never return to any prior object?
left/up100 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 commentedon Mar 7, 2020
I meant that you could extrapolate from the README. The
stubhas the following methods that should return a new object:leftrightupsetValueSIDE NOTE: I absolutely dislike that we use
leftandrightinstead ofprev/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.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.It doesn't if you look at the CI example. It recreates a new
Zippereachup. 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 commentedon Mar 7, 2020
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. :)
When I hear pointer I think of the object property, not the object itself... Ie
Zipper#focusNodeis 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 commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
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
Treethat has aleftandrightbranch. That will clear it up, agreed?I think that was the intention. It also makes it much more likely that people will make everything immutable from the get-go 💯
joshgoebel commentedon Mar 7, 2020
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 commentedon Mar 7, 2020
It doesn't. But for now this is all we can do, because I want to keep following the
problem-specificationsreadme 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
v3this is fixed! So it's only a concern for now anyway. :)