forth: Add canonical data by petertseng · Pull Request #394 · exercism/problem-specifications

petertseng · 2016-10-04T07:53:19Z

Existing tests:

All tracks had the exact same tests, with two exceptions:

First, The "All non-word characters are separators test". In particular,
F# chose not to include the Ogham space mark, and I argue the same in
exercism/haskell#388.

The Ogham space mark adds no additional value (it is just another space
character) and is confusing (it is visually too similar to the minus
sign), so it will not be included.

Second, Rust has a few extras in its tests:

Parsing negative numbers (adds an extra wrinkle since now we must
determine whether a minus is unary or binary)
Extra error cases: +, -, *, / with not enough arguments.
Ensuring that user-defined words are case-insensitive: defines CoUnT
then executes count COUNT
Ensuring that user-defined words execute their definitions in the
right order.
Extra error cases: Incomplete definitions (a : without a ;)

After discussion in #394, we decide that we will only add tests for the
second and fourth of these.

Further, we have changed the expectations from the string representation
of the stack to simply an array/list of values on the stack.

Closes https://github.com/exercism/todo/issues/92

petertseng · 2016-10-04T07:59:43Z

Before merging, I would like discussion on the following.

Should we include test cases for:

Parsing negative numbers? (I recommend no)
Cases for +, -, *, / with not enough arguments? How far should we go? Test everything in the cartesian product of {+, -, *, /} with {0, 1} arguments (8 tests in total)? I feel like we should for completeness, since all our other words have tests for not enough things on the stack.
Ensuring that user-defined words are case-insensitive?
Ensuring that user-defined words execute in the right order? For example that : hi 1 2 ; hi results in 1 2 instead of 2 1? You may think "who would make such a mistake?", and to that I answer "Maybe you should look at my Haskell implementation of this exercise". Yes, I made that mistake. (I'm leaning yes here)
Incomplete word definitions? (I'm leaning no)

In addition, I would to verify this for correctness, since I did a lot by hand.

petertseng · 2016-10-04T08:06:37Z

+      {
+        "description": "multiplication and division",
+        "input": [
+          "2 4 * 3 /"


note that this integer division. maybe that should be called out with a separate test that points this out in its description.

petertseng · 2016-10-04T08:12:17Z

+  "#": [
+    "The cases are split into multiple sections, all with the same structure.",
+    "In all cases, the `expected` key is the string representation of the resulting stack",
+    "if all numbers on it are printed (top-most value on the right), separated by a space."


Time to question assumptions. why is it done this way? It was done this way in exercism/exercism#1188 (the first time this exercise appeared), and I guess everyone else did the same. But why not check an array/list of values on the stack? Instead of "expected": "1 2 3 4 5", why not "expected": [1, 2, 3, 4, 5] ?

Agreed, seems like an array or list would make more sense here.

petertseng · 2016-10-04T08:12:56Z

+  "basic arithmetic": {
+    "cases": [
+      {
+        "description": "addition and subtraction",


why addition and subtraction in one test? Why not separate them? A test with only addition, a test with only subtraction, and then possibly a test combining the two.

I think you're right. Doing them separately first would let people implement this iteratively.

petertseng · 2016-10-04T08:13:14Z

+        "expected": "-1"
+      },
+      {
+        "description": "multiplication and division",


as above, why multiplication and division in one test?

Agreed. We should separate them.

petertseng · 2016-10-04T08:14:11Z

+      {
+        "description": "division by zero results in an error",
+        "input": [
+          "4 2 2 - /"


why not just "4 0 /"?

Is there an advantage of one or the other? Should we have both? Just 4 2 2 - /? Just 4 0 /?

I can't think of a reason to have 4 2 2 - / in addition to or instead of 4 0 /. I vote for just plain 0.

petertseng · 2016-10-07T05:25:08Z

+{
+  "#": [
+    "The cases are split into multiple sections, all with the same structure.",
+    "In all cases, the `expected` key is the string representation of the resulting stack",


for completeness, should clarify: "the resulting stack after executing the Forth program contained in the input" or something to that effect.

kytrinyx · 2016-10-07T22:40:05Z

Parsing negative numbers? (I recommend no)

I also recommend no. I think this exercise has enough meat in it without it.

Cases for +, -, *, / with not enough arguments? How far should we go? Test everything in the cartesian product of {+, -, *, /} with {0, 1} arguments (8 tests in total)? I feel like we should for completeness, since all our other words have tests for not enough things on the stack.

I'm leaning yes on this one.

Ensuring that user-defined words are case-insensitive?

Huh. I can't speak to this as I never did the exercise (and I've never written any forth).

Ensuring that user-defined words execute in the right order? For example that : hi 1 2 ; hi results in 1 2 instead of 2 1? You may think "who would make such a mistake?", and to that I answer "Maybe you should look at my Haskell implementation of this exercise". Yes, I made that mistake. (I'm leaning yes here)

Definitely vote yes on this one :)

Incomplete word definitions? (I'm leaning no)

Meh. Agreed, I don't think we need this, since we're very clear on being a very small subset of forth.

petertseng · 2016-10-08T09:40:16Z

OK.

all expectations are arrays now instead of strings.
arithmetic operator tests split up into own sections
arithmetic operators each get tests that they error if there are zero or one elements on the stack
integer division explicitly called out
divide by zero simplified, it's just 4 0 / now.
add test for user-defined word executing in the right order (countup 1 2 3)

Existing tests: * https://github.com/exercism/xelixir/blob/master/exercises/forth/forth_test.exs * https://github.com/exercism/xfsharp/blob/master/exercises/forth/ForthTest.fs * https://github.com/exercism/xhaskell/blob/master/exercises/forth/test/Tests.hs * https://github.com/exercism/xrust/blob/master/exercises/forth/tests/forth.rs * https://github.com/exercism/xscala/blob/master/exercises/forth/src/test/scala/ForthTest.scala All tracks had the exact same tests, with two exceptions: First, The "All non-word characters are separators test". In particular, F# chose not to include the Ogham space mark, and I argue the same in exercism/haskell#388. The Ogham space mark adds no additional value (it is just another space character) and is confusing (it is visually too similar to the minus sign), so it will not be included. Second, Rust has a few extras in its tests: * Parsing negative numbers (adds an extra wrinkle since now we must determine whether a minus is unary or binary) * Extra error cases: +, -, *, / with not enough arguments. * Ensuring that user-defined words are case-insensitive: defines `CoUnT` then executes `count COUNT` * Ensuring that user-defined words execute their definitions in the right order. * Extra error cases: Incomplete definitions (a `:` without a `;`) After discussion in #394, we decide that we will only add tests for the second and fourth of these. Further, we have changed the expectations from the string representation of the stack to simply an array/list of values on the stack. Closes https://github.com/exercism/todo/issues/92

petertseng · 2016-10-08T10:23:18Z

In addition, I would to verify this for correctness, since I did a lot by hand.

OK. https://github.com/petertseng/x-common/blob/verify/exercises/forth/verify.rb

petertseng · 2016-10-17T05:12:29Z

All right, all comments addressed, and nobody else seems to have any, so I exercise my judgment and say it's good.

petertseng · 2017-06-03T05:19:06Z

+      {
+        "description": "all non-word characters are separators",
+        "input": [
+          "1\u00002\u00133\n4\r5 6\t7"


Garbage. This \u0013 is a "device control 3". The original exercism/exercism#1188 meant for this to be \u0001, "start of heading" (whatever that is).

My fault. Sorry.

Questioning the assumptions: Why must the output be a string? Reasons I thought of: * It was this way in the original version of the exercise: exercism/exercism#1188 * But, we have no obligation to follow that. * It matches the fact that we have strings as inputs. * But, it is probably more work for the student to create a string. * It can be argued that it's easier to check the vector. Previous discussions: exercism/problem-specifications#394 (comment) exercism/haskell#412 This doesn't get this track all the way to forth 1.0.0, but is a useful step there.

Automatically generate "binary" exercise tests

petertseng commented Oct 4, 2016

View reviewed changes

petertseng commented Oct 7, 2016

View reviewed changes

petertseng merged commit 044ad55 into exercism:master Oct 17, 2016

petertseng deleted the forth-json branch October 17, 2016 05:12

This was referenced Nov 6, 2016

forth: Expect a list rather than a string or Text exercism/haskell#412

Merged

forth: Add new tests from x-common exercism/haskell#413

Merged

petertseng mentioned this pull request Nov 29, 2016

Should we have any sort of automatic verification of canonical-data.json content? #460

Closed

petertseng mentioned this pull request May 19, 2017

forth: return Vec<Value>, not String, from stack exercism/rust#294

Merged

petertseng commented Jun 3, 2017

View reviewed changes

emcoding pushed a commit that referenced this pull request Nov 19, 2018

Add test generator for "binary" exercise (#394)

f319c72

Automatically generate "binary" exercise tests

Uh oh!

Conversation

petertseng commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented Oct 7, 2016

Uh oh!

petertseng commented Oct 8, 2016

Uh oh!

petertseng commented Oct 8, 2016

Uh oh!

petertseng commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

petertseng commented Oct 4, 2016 •

edited

Loading