variable-length-quantity: create test case generator #674

leenipper · 2017-05-18T04:10:58Z

Generate two test case arrays, one for encode and
one for decode, from the canonical-data.json.

Update example solution to pass the tests.
The API changed to use slices in order to match
the canonical-data.json test cases.
Retain the old functions but rename to decodeInt and encodeInt
and utilize these as helpers to support the new slice based API.

Update test program to use generated test case arrays.
Put FAIL and PASS in test result output.

Increment testVersion and targetTestVersion in
example solution and test program.

petertseng · 2017-05-18T04:46:49Z

exercises/variable-length-quantity/.meta/gen.go

+	Description string
+	Property    string      // "encode" or "decode"
+	Input       []uint32    // supports both []byte and []uint32 in JSON.
+	Expected    interface{} // supports []byte, []uint32, or null in JSON.


two thing stand out to me:

I was a bit surprised: I thought that since nil is assignable to []uint32 that []uint32 can be used here

I feel like the schema is not doing well for us. In the old way, where encode and decode are keys, we can have a different struct for each kind of case. But since now we can only tell by looking at property, we have to make our OneCase struct a union of all possible cases. Since you have written more generators than I have, I would ask you: How much does this bother you? And I guess more importantly, does it bother enough that it's worth proposing a change?

[]uint32 can be used here

Actually, I think you are right. I jumped to the more general interface{}, likely because I had to previously for a different generator.

In the old way, where encode and decode are keys, we can have a different struct for each kind of case.

This made it easier, I agree.

does it bother enough that it's worth proposing a change?

Well, I've not thought much about what the change might be. But I do like simplicity, and the newer approach in the JSON is adding complexity to the generators. Perhaps there is a compromise. Do you know the rational for the new way in the JSON ?

Do you know the rational for the new way in the JSON ?

I didn't know off the top of my head, so I went searching. The clearest explanation is probably: exercism/problem-specifications#336 (comment) :

In most of the test suites with more than one type of test, the test's type is encoded
in a property-key, in an object describing a test group.

There are two problem with that approach:

It mixes two different concepts regarding the tests: grouping and identification

It doesn't allow nesting of test groups. That would be nice to have, but is not really needed.

It doesn't allow grouping of test of different types, which would be really great.

Moving the test type near the test data, we solved all the above problem easily. It is theoretically sound and adds functionality.

I wonder if it would be too much to ask to have the keys again.

I'm wondering what other statically-typed language has generators. There's https://github.com/exercism/xscala/tree/master/testgen/src/main/scala and https://github.com/exercism/xocaml/tree/master/tools/test-generator, might need to take a look at what they're doing.

I considered another alternative, which is... can we deal with this on the Go side without needing a change in x-common? I tried it in #677.

Yes, I've been looking at that one.

leenipper · 2017-05-19T01:50:43Z

I pushed an update which has a simpler generator.

petertseng · 2017-05-21T04:43:27Z

exercises/variable-length-quantity/example.go

-			return x, n + 1
+			return x, n + 1, nil
+		} else if n == len(buf)-1 {
+			// error case


since this is the last iteration, this isn't necessary, is it?

I just checked - the example solution still passes the tests even if these lines are removed, so I would suggest removing them.

Yes. Will remote those lines.

petertseng

Doesn't need to be in this PR, but at this point I wonder why the "size" is necessary for the decode cases. Now that we require the decoding to decode all the bytes OR signal an error if it was not possible to decode them all, the size will always equal the input slice, and it doesn't seem useful to return it.

I kind of wonder whether it was possible to split this up into several commits, for example one commit that adds multi-int decoding to the example, but I guess it's hard since the test expectations are also changing with it.

So there is only one thing I would change in this: Removing the unnecessary lines from the for loop in the example (or let me know if I'm wrong and they are actually necessary)

petertseng · 2017-05-21T04:53:00Z

exercises/variable-length-quantity/.meta/gen.go

@@ -0,0 +1,115 @@
+// +build ignore


we don't need to build ignore generators anymore, now that they're in the separate directory, right? They originally were to stop them from being caught when we go test, but now no longer necessary.

Doesn't need to be in this PR, maybe I'll make a separate one to remove them.

Ok, can be done with one sweeping commit on all the .meta/gen.go files easily later.

I'll make an issue so I don't forget (or if someone else wants to take it)

ok now that #682 is merged, gonna ask you to remove it from this one

petertseng · 2017-05-21T04:55:17Z

exercises/variable-length-quantity/example.go

-			return x, n + 1
+			return x, n + 1, nil
+		} else if n == len(buf)-1 {
+			// error case


I just checked - the example solution still passes the tests even if these lines are removed, so I would suggest removing them.

petertseng

OK. I still think that returning the number of bytes is something I don't understand, but I suppose it can go separately, since it existed before this PR so this PR doesn't need to be concerned with removing it

another thought: 0-length array in test data meaning error, is it going to be confusing at all?

leenipper · 2017-05-22T04:14:18Z

Well, I did wonder about the 0-length array, whether it was clear enough. Do you think it is worth adding an errorExpected bool for clarity in the test data ? Will be fairly easy to do.

petertseng · 2017-05-22T05:51:15Z

I would add it - I'm looking at the example of https://github.com/exercism/xgo/blob/master/exercises/largest-series-product/cases_test.go where we could find out via expected < 0 whether it's an error, but we still add an ok boolean anyway.

However, just to make sure I'm not falling to confirmation bias, I looked for a counterexample, and I found it in Forth: https://github.com/exercism/xgo/blob/master/exercises/forth/cases_test.go#L15 there is a comment there:

// nil slice indicates error expected.

So it looks like there are two possible ways of making it clearer, so we have a choice.

leenipper · 2017-05-22T11:15:12Z

Ok. I chose to add the nil slice comment, and I updated test program to compare against nil as well.

petertseng · 2017-05-22T16:31:07Z

I just remembered something... forgot to check that err is an error? (#275)

Maybe we should only check it in the first few exercises that have error and not worry about it for the rest.

Generate two test case arrays, one for encode and one for decode, from the canonical-data.json. Update example solution to pass the tests. The API changed to use slices in order to match the canonical-data.json test cases. Retain the old functions but rename to decodeInt and encodeInt and utilize these as helpers to support the new slice based API. Update test program to use generated test case arrays. Put FAIL and PASS in test result output. Increment testVersion and targetTestVersion in example solution and test program.

petertseng

everything seems to be in order.

leenipper · 2017-05-23T01:06:53Z

Accidental close earlier. Sorry.
I updated the PR commit without the +build tag and with the var _ error check.

Since it was introduced in #322, the decode function returns the number of bytes decoded. The creator of the exercise mentioned that it was to be consistent with the standard library's API. Go standard library example for returning number of bytes: https://golang.org/pkg/io/#Writer Now, since #674, it is required that the entire input array be decoded, or else it is an error. Now the number of bytes decoded doesn't make much sense to return, since now it is known that it will always be the length of the input. Go standard library example for not returning number of bytes (decoding everything): https://golang.org/pkg/encoding/json/#Unmarshal testVersion -> 4 Closes #683

petertseng reviewed May 18, 2017

View reviewed changes

leenipper force-pushed the variable-length-quantity-add-generator branch from e7ee588 to d07d9ba Compare May 19, 2017 01:45

petertseng reviewed May 21, 2017

View reviewed changes

leenipper force-pushed the variable-length-quantity-add-generator branch from d07d9ba to 5fd7847 Compare May 22, 2017 01:29

petertseng approved these changes May 22, 2017

View reviewed changes

leenipper force-pushed the variable-length-quantity-add-generator branch from 5fd7847 to 618e4f6 Compare May 22, 2017 11:11

leenipper closed this May 23, 2017

leenipper force-pushed the variable-length-quantity-add-generator branch from 618e4f6 to 925762f Compare May 23, 2017 00:57

leenipper reopened this May 23, 2017

petertseng approved these changes May 23, 2017

View reviewed changes

leenipper merged commit 6325f89 into exercism:master May 23, 2017

petertseng mentioned this pull request May 23, 2017

Write generators for all exercises #605

Open

48 tasks

leenipper deleted the variable-length-quantity-add-generator branch May 23, 2017 01:12

This was referenced May 23, 2017

variable-length-quantity: I don't think returning number of bytes decoded is necessary #683

Closed

variable-length-quantity: Decode: don't return/test length #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

variable-length-quantity: create test case generator #674

variable-length-quantity: create test case generator #674

leenipper commented May 18, 2017

petertseng May 18, 2017 •

edited

Loading

leenipper May 18, 2017

petertseng May 18, 2017 •

edited

Loading

petertseng May 21, 2017

leenipper May 22, 2017

leenipper commented May 19, 2017

petertseng May 21, 2017

petertseng May 21, 2017

leenipper May 22, 2017

petertseng left a comment

petertseng May 21, 2017

leenipper May 22, 2017

petertseng May 22, 2017

petertseng May 22, 2017 •

edited

Loading

petertseng May 21, 2017

petertseng left a comment

leenipper commented May 22, 2017

petertseng commented May 22, 2017

leenipper commented May 22, 2017

petertseng commented May 22, 2017

petertseng left a comment

leenipper commented May 23, 2017

variable-length-quantity: create test case generator #674

variable-length-quantity: create test case generator #674

Conversation

leenipper commented May 18, 2017

petertseng May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leenipper commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng left a comment

Choose a reason for hiding this comment

leenipper commented May 22, 2017

petertseng commented May 22, 2017

leenipper commented May 22, 2017

petertseng commented May 22, 2017

petertseng left a comment

Choose a reason for hiding this comment

leenipper commented May 23, 2017

petertseng May 18, 2017 •

edited

Loading

petertseng May 18, 2017 •

edited

Loading

petertseng May 22, 2017 •

edited

Loading