Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diamond: Implement canonical-data.json #556

Closed
kytrinyx opened this issue Feb 13, 2017 · 12 comments
Closed

diamond: Implement canonical-data.json #556

kytrinyx opened this issue Feb 13, 2017 · 12 comments

Comments

@kytrinyx
Copy link
Member

We want to have a standard set of test inputs and outputs for each exercise to
make it easier to port them to new languages, as well as to help keep the
exercises in sync.

The Diamond exercise can be found in the ./exercises/diamond/ directory.

The step-by-step instructions for how to compile a canonical-data.json file is
described in this section of the contributing guide.

See http://exercism.io/contribute/canonical-data/diamond for the up-to-date list of
language tracks that have an implementation of the Diamond exercise.

The tracking issue has more context.

@stkent
Copy link
Contributor

stkent commented Feb 13, 2017

I'll happily pick this issue up: I recently built the exercise for the Java track and so surveyed the existing implementations.

@stkent stkent self-assigned this Feb 13, 2017
@petertseng
Copy link
Member

And while we're on the discussion, I'll go ahead and say it can be interesting to encourage tracks to use some property-based testing rather than example-based testing, as indicated in the discussion of the PR that introduced the exercise: #191 (comment)

@stkent
Copy link
Contributor

stkent commented Feb 13, 2017

Mmm, interesting. I am not sure how best to capture "write tests according to this philosophy if possible, but if not according to this other philosophy" in the existing canonical data structure. Any suggestions?

@petertseng
Copy link
Member

Me neither! Adding another facet to this problem, the property-based philosophy is at best unnatural to encode into JSON. I imagine that at best it would only be able to include something like the bulleted list in the README and then it is up to the individual tracks to implement the actual checks. In contrast, example-based tests are very easy to encode.

Indeed, in the domino problem, there is also the choice between example-based testing (true or false) versus property-based testing (https://github.com/exercism/x-common/blob/master/exercises/dominoes/canonical-data.json#L27). The best I came up with at the time was to leave a very lengthy comment in the file.

Even today I do not have a better suggestion. It may be that the example-based tests can go in the JSON file, with a comment saying "try out property-based testing".

@ErikSchierboom
Copy link
Member

Interesting problem. What I did when I created this exercise, is to setup the tests such that they could be solved using regular example based tests (using a couple of standard input letters), but that they are actually perfectly suited to property-based testing.

For example, I have a test Diamond has square shape, which is clearly a property test. For the F# track, I then chose not to use an actual full-fledged property-based testing framework, but to simply use parameterized tests with the parameterized values being the characters from 'a' to 'z'.

Maybe the best we can do is say something about how track implementers could try to write property-based tests when implementing it?

@stkent
Copy link
Contributor

stkent commented Feb 26, 2017

Existing implementations:

xcsharp: property-based: A-Z
xecmascript: value-based: A, C, E
xelixir: value-based: A, C, E
xfsharp: property-based: A-Z
xgo: property-based: A-Z
xjava: value-based: A, B, C, E, Z
xjavascript: value-based: A, C, E
xlua: value-based: A, B, C, E, H
xpython: value-based: A, C, E
xruby: value-based: A, C, E

@stkent
Copy link
Contributor

stkent commented Feb 26, 2017

Observations:

  • 3/10 implementations use property-based testing; 7/10 use value-based
  • 4/10 implementations test both boundaries (A and Z)
  • 5/10 implementations test exactly the cases from the problem description, A, C, and E, and therefore never test a diamond with even side length

@stkent
Copy link
Contributor

stkent commented Feb 26, 2017

I have to admit, this problem was my first exposure to property-based testing! It's an interesting approach. I'm not sure that the existing problem description, which describes the desired output in terms of its properties, is ideal - it doesn't really suit languages that currently use value-based testing. I'd be interested in making the description more "typical", and leaving it up to each track to decide whether this is an appropriate place to introduce the concept of and tooling around property-based testing. In terms of the canonical test data; I'm on board with @petertseng's suggestion to describe typical value-based tests, and use the canonical data's description block as a way to nudge implementors towards a property- based approach if they are feeling adventurous.

@stkent
Copy link
Contributor

stkent commented Feb 26, 2017

If using value-based testing, the following cases seem sufficient:

  • A (degenerate case: only one 'A' row)
  • B (degenerate case: no rows with three distinct groups of spaces)
  • C (smallest non-degenerate case for an "odd" diamond side length)
  • D (smallest non-degenerate case for an "even" diamond side length)
  • Z (boundary case: largest possible diamond)

@ErikSchierboom
Copy link
Member

@stkent I think having those five test cases should indeed be sufficient. As long as we state somewhere in the comments that this exercise's test suite could also use property-based testing, I think it very much makes sense to use the value-based tests for the canonical data.

@stkent
Copy link
Contributor

stkent commented Feb 26, 2017

@ErikSchierboom awesome; how does the comment in #612 look to you?

@ErikSchierboom
Copy link
Member

@stkent Nothing to add, looks perfect to me! Great work. 👍

emcoding pushed a commit that referenced this issue Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants