zipper: add canonical data #934

HarrisonMc555 · 2017-10-09T07:44:24Z

Closes #593.

Of the five tracks that had already implemented this exercise, five had the exact same test cases, and the other two had the same test cases plus a few more (identical) test cases. I opted to simply list all of the test cases I found in the order they were in.

I ran the test against the canonical-schema.json and it said the file is valid.

HarrisonMc555 · 2017-10-09T07:49:58Z

For history, these are the tracks that had implemented this exercise when I created this branch:

C# (common)
F# (common)
Haskell (common)
OCaml (common)
Scala (common)
Erlang (augmented)
Elixir (augmented)

By "common", I mean that the track had the same test cases as every other track. By "augmented", I mean the track had additional test cases. Both "augmented" tracks had the same additional test cases.

NobbZ · 2017-10-09T09:54:21Z

Erlang (augmented)

Elixir (augmented)

[...] Both "augmented" tracks had the same additional test cases.

I used the elixir tests as base when I created the erlang version. It was the most logical thing for me, because both languages share the same virtual machine and therefore the same base types.

Can you tell which cases are the additional ones?

HarrisonMc555 · 2017-10-09T16:00:40Z

Here you go:

Categorizing Test Cases

Common

data is retained
left, right and value
dead end
tree from deep focus
setValue
setLeft with Just
setRight with Nothing
different paths to same zipper

Augmented

data is retained
left, right and value
dead end
tree from deep focus
traversing up from top
left, right, and up
set_value
set_value after traversing up
set_left with leaf
set_right with nil
set_right with subtree
set_value on deep focus

Additional

traversing up from top
left, right, and up
set_value after traversing up
set_right with subtree
set_value on deep focus

Missed

different paths to same zipper

Work to be done

I didn't realize until now that there was one test case that was included in the "common" set of tracks but not in the "augmented" tracks, and that is "different paths to same zipper".

In my current version of canonical_data.json, the "expected" value can either be a value (int) or a tree. However, there are test cases that expect a zipper. The only one I've included so far expected the zipper to be null, so it wasn't really an issue. However, this last test case poses two challenges:

How do we encode a zipper, which includes data for both the tree it is traversing and the current position inside the tree?
How do we encode an expected value that is not just a static value (whether int, tree, or zipper) but instead a sequence of operations to perform, just like what we're testing against?

OCaml specifically differentiates between comparing ints, trees, and zippers. Should I make the expected value an object, with a "type" parameter specifying whether it is an int, tree, or zipper?

Also, should the "expected" value be encoded with either just a static value OR an initial tree and a sequence of operations?

I would love any input.

Example

The way I see it, explicit type encodings would be the most consistent and useful. The only part about this that I don't totally feel comfortable with is the difference between value based encoding of zippers and operation based encoding of zippers...is the fact that one has a "value" member and one has "initialTree" and "operations" members make them different enough to make it easy to parse?

Tree

"expected": {
  "type": "tree",
  "value": {
    "value": 1,
    "left": null,
    "right": null
  }
}

Int

"expected": {
  "type": "int",
  "value": 3
}

Zipper

Value based encoding

"expected": {
  "type": "zipper",
  "value": null
}

Operations based encoding

"expected": {
  "type": "zipper",
  "initialTree": {
    "value": 1,
    "left": null,
    "right": null
  },
  "operations": [
    {
      "operation": "from_tree"
    },
    {
      "operation": "right"
    }
  ]
}

Insti · 2017-10-09T20:09:02Z

I'll recuse myself from this one due to lack of familiarity with the exercise, but I'd like to say a big thankyou to you @HarrisonMc555 for tackling this! ⭐ ❤️ ⭐️

petertseng

Nice, you'll have to wait a while for me to write the code, but I'll try to get it done within the next few hours.

petertseng · 2017-10-10T01:54:30Z

exercises/zipper/canonical-data.json

+      "cases": [
+        {
+          "description": "data is retained",
+          "property": "property",


I understand that there is no real good name for this property, but I hope there is at least a way to sneak a little bit of usefulness in here rather than leaving it as "property".

What of the suggestion of https://github.com/exercism/problem-specifications/blob/master/exercises/circular-buffer/canonical-data.json ("run") or https://github.com/exercism/problem-specifications/blob/master/exercises/react/canonical-data.json ("react")? Are they suitable for use here?

I meant to decide on a good name but forgot 😅
Do you have any suggestions?

Would we possibly use the property to categorize what kind of test we're running, i.e. function(input) == output vs. function(input) == function(input)?

petertseng · 2017-10-10T03:48:30Z

exercises/zipper/canonical-data.json

+  "cases": [
+    {
+      "description": "Creating a zipper for a binary tree",
+      "cases": [


far as I can tell the only reason for this second level of nesting under cases is to attach a description to this entire group? I feel like we could do without, and just one level of nesting under cases. Yes? No?

This...was a mistake. I think I copied another exercise as a starting point and didn't realize this was happening. I just changed it.

petertseng

The from_tree operation only makes sense when given a tree (initialTree). And it appears that initialTree is not used anywhere else.

Therefore, I ask:

What about making the initialTree instead a tree key to those operations whose type are from_tree?

Alternatively, given that from_tree is the first operation of of all the lists of operations:

Leave initialTree in place but remove from_tree. All other operations need a zipper to act on, so it's implicit that that zipper is the zipper resulting from using from_tree on initialTree.

petertseng · 2017-10-10T04:28:55Z

exercises/zipper/canonical-data.json

+          }
+        },
+        {
+          "description": "set_right with subtree",


"set_right with subtree" case currently is missing a to_tree as its last operation. The operations result in a zipper, but the expected value is a tree.

petertseng · 2017-10-10T04:32:55Z

OK, I managed to finish https://github.com/petertseng/exercism-problem-specifications/blob/verify-zipper/exercises/zipper/verify.rb . I have reported all problems it found. One test is missing a to_tree is the only problem. Other suggestions might help make things clearer, but don't affect correctness.

HarrisonMc555 · 2017-10-11T21:21:16Z

Leave initialTree in place but remove from_tree. All other operations need a zipper to act on, so it's implicit that that zipper is the zipper resulting from using from_tree on initialTree.
I think I like this the best. It's true that the rest of the operations implicitly are using the zipper produced by the previous operation, so this would make it more consistent.

HarrisonMc555 · 2017-10-11T21:24:07Z

I'm still trying to decide on what to use for the "property" key. I can either use an arbitrary string (not much better than "property": "property") or try to use it to encode what kind of test I'm running.

Would something like expectedValue and sameResultFromOperations be any good? The idea being that the first is function(input) == output and the latter is function1(input1) == function2(input2)? Or is that not what the "property" key is for?

petertseng · 2017-10-11T22:29:46Z

git show -w b21648f56841289a94d22edb93123118eb999edd looks as I expect it to.

Would something like expectedValue and sameResultFromOperations be any good? The idea being that the first is function(input) == output and the latter is function1(input1) == function2(input2)? Or is that not what the "property" key is for?

Looks like an acceptable use of property. To me differing property means the meaning of the keys is different, and this is what you are proposing.

petertseng · 2017-10-11T22:34:06Z

https://github.com/petertseng/exercism-problem-specifications/blob/verify-zipper/exercises/zipper/verify.rb has been updated (remove nesting, and use initialTree as the initial zipper, without the from_tree operation) and tests pass.

petertseng

given that verify.rb says it is OK, I would be happy with this file as-is. You are free to ask for another look if you would like something done with property, of course.

HarrisonMc555 · 2017-10-11T22:54:23Z

Ok I changed the property values. I think I'm good to go. Thanks @petertseng!

petertseng

Agreed. This property separation makes sense. If it's expectedValue, then you check that you get the value in expected -> value. If it's sameResultFromOperations you check that the zipper is the same as the one from initialTree and operations in expected. Nice work. I am ready for this.

(I advise squashing)

Use explicit type encoding, add another test case Remove unnecessary `cases` nesting Fix bug in test case In the "set_right with subtree" test case, it was missing "to_tree" as the final operation. Remove "from_tree" operations Use property to encode type of test

HarrisonMc555 · 2017-10-12T00:33:36Z

Done, ready to merge.

petertseng · 2017-10-12T08:57:07Z

Great, thanks.

I hope I am not remiss in merging. I figure that if anyone wanted to chime in, they've had 72 hours to do so.

petertseng reviewed Oct 10, 2017

View reviewed changes

petertseng approved these changes Oct 11, 2017

View reviewed changes

HarrisonMc555 force-pushed the zipper-canonical-data branch from fa44378 to e014a5d Compare October 12, 2017 00:32

petertseng merged commit 38cab15 into exercism:master Oct 12, 2017

HarrisonMc555 deleted the zipper-canonical-data branch October 12, 2017 20:50

This was referenced Apr 11, 2018

diffie-hellman: Add canonical data #1224

Merged

Schema optimises for testing f(inputs) = output. What of other types of tests? #1225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zipper: add canonical data #934

zipper: add canonical data #934

HarrisonMc555 commented Oct 9, 2017

HarrisonMc555 commented Oct 9, 2017

NobbZ commented Oct 9, 2017

HarrisonMc555 commented Oct 9, 2017 •

edited

Insti commented Oct 9, 2017

petertseng left a comment

petertseng Oct 10, 2017

HarrisonMc555 Oct 11, 2017

petertseng Oct 10, 2017

HarrisonMc555 Oct 11, 2017

petertseng left a comment •

edited

petertseng Oct 10, 2017 •

edited

petertseng commented Oct 10, 2017 •

edited

HarrisonMc555 commented Oct 11, 2017

HarrisonMc555 commented Oct 11, 2017 •

edited

petertseng commented Oct 11, 2017

petertseng commented Oct 11, 2017

petertseng left a comment

HarrisonMc555 commented Oct 11, 2017

petertseng left a comment •

edited

HarrisonMc555 commented Oct 12, 2017

petertseng commented Oct 12, 2017

zipper: add canonical data #934

zipper: add canonical data #934

Conversation

HarrisonMc555 commented Oct 9, 2017

HarrisonMc555 commented Oct 9, 2017

NobbZ commented Oct 9, 2017

HarrisonMc555 commented Oct 9, 2017 • edited

Categorizing Test Cases

Common

Augmented

Additional

Missed

Work to be done

Example

Tree

Int

Zipper

Value based encoding

Operations based encoding

Insti commented Oct 9, 2017

petertseng left a comment

Choose a reason for hiding this comment

petertseng Oct 10, 2017

Choose a reason for hiding this comment

HarrisonMc555 Oct 11, 2017

Choose a reason for hiding this comment

petertseng Oct 10, 2017

Choose a reason for hiding this comment

HarrisonMc555 Oct 11, 2017

Choose a reason for hiding this comment

petertseng left a comment • edited

Choose a reason for hiding this comment

petertseng Oct 10, 2017 • edited

Choose a reason for hiding this comment

petertseng commented Oct 10, 2017 • edited

HarrisonMc555 commented Oct 11, 2017

HarrisonMc555 commented Oct 11, 2017 • edited

petertseng commented Oct 11, 2017

petertseng commented Oct 11, 2017

petertseng left a comment

Choose a reason for hiding this comment

HarrisonMc555 commented Oct 11, 2017

petertseng left a comment • edited

Choose a reason for hiding this comment

HarrisonMc555 commented Oct 12, 2017

petertseng commented Oct 12, 2017

HarrisonMc555 commented Oct 9, 2017 •

edited

petertseng left a comment •

edited

petertseng Oct 10, 2017 •

edited

petertseng commented Oct 10, 2017 •

edited

HarrisonMc555 commented Oct 11, 2017 •

edited

petertseng left a comment •

edited