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

leap: Make the test descriptions consistent #453

Merged
merged 1 commit into from
Nov 28, 2016

Conversation

guntbert
Copy link
Contributor

@guntbert guntbert commented Nov 27, 2016

description adheres to the schema: leap year / standard year, then either the century or the year number itself.

No testcases have been altered/added/removed.

Related: exercism/discussions#104

 description adheres to the schema: leap year / standard year, then **either** the century **or** the year number itself
@Insti Insti changed the title Make the test descriptions consistent leap: Make the test descriptions consistent Nov 28, 2016
@Insti
Copy link
Contributor

Insti commented Nov 28, 2016

@guntbert please read this comment about test descriptions.

The descriptions you have here are OK, but they don't really describe WHY those particular years are good numbers to test.

What:

      {
          "description": "leap year in twentieth century",
          "input": 1996,
          "expected": true
       },

Why:

      {
          "description": "a leap year is evenly divisible by 4",
          "input": 1996,
          "expected": true
       },

@guntbert
Copy link
Contributor Author

@Insti I agree that we should describe the why instead of the what. But I have my doubts about your proposal. Case in question:

"description": "leap year in twentieth century",

says

"1996 is chosen as an arbitrary leap year from the twentieth century"

whereas

"description": "a leap year is evenly divisible by 4",

merely restates the facts given in description.md

I think most of the test cases should be reevaluated, asking if they contribute anything useful - but I did not want (as a newcomer) to overthrow the complete test suite and thus just changed the descriptions.

If you think this should be done right now I'll do it :-)

@Insti
Copy link
Contributor

Insti commented Nov 28, 2016

I did not want (as a newcomer) to overthrow the complete test suite and thus just changed the descriptions.

That is certainly a good approach. 👍

I think most of the test cases should be reevaluated, asking if they contribute anything useful ...
... If you think this should be done right now I'll do it :-)

That would be good.

@Insti
Copy link
Contributor

Insti commented Nov 28, 2016

I agree that we should describe the why instead of the what. But I have my doubts about your proposal. Case in question:
"description": "leap year in twentieth century",
says
"1996 is chosen as an arbitrary leap year from the twentieth century"
whereas
"description": "a leap year is evenly divisible by 4",
merely restates the facts given in description.md

This, to me, is a feature. The rules should be encoded in and described by the tests, description.md provides 'color' and allows a written introduction/specification of the problem.

"description": "leap year in twentieth century",
says
"1996 is chosen as an arbitrary leap year from the twentieth century"

Why is it relevant that it is in the twentieth century?
How do we know it is a leap year?

How about this made up (counter) example:

{
   "description": "floopy year in twentieth century",
   "input": 1969,
   "expected": true
},

What does this tell you about floopy years?

(Sorry about the mangled quote formatting at the top.)

@Insti
Copy link
Contributor

Insti commented Nov 28, 2016

@guntbert did you want to continue this PR for the updated tests or shall we merge this one and you can make a new one for the updated tests?

@guntbert
Copy link
Contributor Author

@Insti as I am a bit pressed for time right now (several tests to grade :-)) I'd rather take a fresh approach with a new PR. Fine if you merge this one but I agree that it is suboptimal.

@Insti Insti merged commit 1fbedca into exercism:master Nov 28, 2016
@Insti
Copy link
Contributor

Insti commented Nov 28, 2016

It might not be perfect yet, but it is an improvement.

Thanks for your contribution @guntbert. ❤️

@guntbert guntbert deleted the test-descr-consistent branch December 1, 2016 15:57
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.

2 participants