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

Add descriptions to Roman numerals canonical data. #451

Closed
wants to merge 1 commit into from

Conversation

stevejb71
Copy link
Contributor

Descriptions could be worked out programatically (or by a human), but:

  1. this entails more work for an automated test generator, which is not needed elsewhere as most (if not all) other data files provide descriptions.
  2. descriptions won't be consistent across streams

Descriptions could be worked out programatically (or by a human), but:
1. this entails more work for an automated test generator, which is not needed elsewhere as most (if not all) other data files provide descriptions.
2. descriptions won't be consistent across streams
@Insti
Copy link
Contributor

Insti commented Nov 26, 2016

@stevejb71 thanks for your work on this, it's good for all test data files to have descriptions.

However, the descriptions need to be useful. As you've said yourself these descriptions could be derived from information already in the file, so they are not adding any value, and cross stream consistency of error messages is a very unimportant thing.

A description should explain what a test is testing and why this test is important.

Have a look at bowling

{
      "description": "a spare in the last frame gets a one roll bonus that is counted once",
      "rolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 7],
      "expected": 17
},

This is a test to catch a common implementation bug that would cause the bonus roll to be counted twice.

And note how that is different from:

      "description": "when rolling [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 7] the game score is 17"

Roman numerals is a hard, because it's easy to fall into the trap you have and just make a simple in -> out mapping.

Think about why each number is being tested.
Why is 1 there?
Why is 7 NOT there?
Why is there no test for 10?

If you do this process fully you'll find test cases that are missing and test cases that are redundant, but at the end you'll have a set of tests with descriptions that do usefully and clearly describe the problem.

Copy link
Contributor

@Insti Insti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Descriptions should explain what a test is testing and why this test is important. Not just echo the input and output values.

@stevejb71
Copy link
Contributor Author

I'll take a look, it's a little hard to form good concise descriptions of these tests.

@Insti Insti mentioned this pull request Dec 8, 2016
@ErikSchierboom
Copy link
Member

@stevejb71 A little bump :) Are you still interested in working on this PR?

Coming up with good descriptions can be hard, but I think it is possible here. There is a very nice post on the F# for fun and profit website on implementing Roman Numerals in F#. In that post, several requirements are listed:

  • Five in a row of any digit is not allowed
  • Some digits are allowed in runs of up to 4. They are I,X,C, and M. The others (V,L,D) can only appear singly.
  • Some lower digits can come before a higher digit, but only if they appear singly. E.g. "IX" is ok but "IIIX" is not.
  • But this is only for pairs of digits. Three ascending numbers in a row is invalid. E.g. "IX" is ok but "IXC" is not.
  • A single digit with no runs is always allowed

Perhaps you can use those requirements as an inspiration for test descriptions (and test data)?

@stevejb71
Copy link
Contributor Author

There look to be some good ideas in that article such should help me get started.

May take a few weeks, I've got a holiday coming up shortly.

@ErikSchierboom
Copy link
Member

@stevejb71 Good luck with it. And enjoy your holiday :)

@rbasso
Copy link
Contributor

rbasso commented Mar 11, 2017

Hi,

We're mostly finishing #625, and roman-numerals is one of the remaining problem to adapt to the new JSON Schema. The major problem with it is the lack of descriptions, which are enforced now.

Could we merge this the way it is to allow compliance and later fix it as someone comes up with better descriptions?

@petertseng
Copy link
Member

petertseng commented Mar 11, 2017

Erm well I can offer my descriptions that I tried to cook up for this special occasion just now, but it's a bit unfortunate because for example the description for the 93 test implies it is only testing 90, but it is testing 90 + 3.

It's in a PR now so y'all can comment on it if y'all care.

petertseng added a commit that referenced this pull request Mar 11, 2017
We'd like some descriptions so that roman-numerals can be brought into
compliance with the schema, as in #625.

Closes #451 via replacement
@rbasso rbasso deleted the AddRomanNumeralsDescriptions branch March 12, 2017 09:29
emcoding pushed a commit that referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants