leap: Rewrite the test cases and their descriptions #463

guntbert · 2016-12-05T20:49:07Z

only test cases remain that are necessary
they have a clear description to help providing good names for tests

related: #462

stevejb71 · 2016-12-06T00:18:07Z

exercises/leap/canonical-data.json

-         "description": "leap year two thousand",
-         "input": 2000,
-         "expected": true
+         "description": "year before validity of Gregorian Calendar (no leap years at all): standard year",


I don't think Gregorian calendar checks should be introduced in an exercise as early as this.

Also, 1581 is not a leap year by the check on divisibility by 4.

Do I understand correctly that you would omit the boundary check completely, although there were no leap years before 1582 - thus only relying on the three rules?

I would think that is better for this exercise. It is the first one on some tracks, better to keep it simple.

stevejb71 · 2016-12-06T00:20:08Z

exercises/leap/canonical-data.json

-      {
-         "description": "even standard year in twentieth century",
-         "input": 1998,
+         "description": "year not divisible by 4: standard year",


Where does the term standard year come from? I see common year on Wikipedia. I would think just "not leap year" would be best though.

"standard year" was not from me, "not a leap year" is fine by me.

Agreeing with Wikipedia: "A year that is not a leap year is called a common year." is a good idea.

kytrinyx · 2016-12-06T16:50:05Z

Would you mind amending the commit so that the slug of the exercise is included? It makes it a lot easier to dig through the logs later.

guntbert · 2016-12-06T17:53:13Z

@kytrinyx sorry I have no idea what the "slug of the exercise" might be.

Insti · 2016-12-06T18:03:37Z

exercises/leap/canonical-data.json

-         "description": "standard year in nineteenth century",
-         "input": 1900,
-         "expected": false
+         "description": "year divisible by 4, not divisible by 100: leap year",


The "not divisible by 100" restriction is not relevant here.

Well, only years that are not divisible by 100 are leap years, aren't they? So IMO "year divisible by 4" alone is not enough, we should also state that the exception does not apply.

The only rule that applies here is 'year divisible by 4' you do not need any other rules to determine if 2016 is a leap year.

The 100 rule is only relevant when we have a year that is divisible by 100, which 2016 is not.

Why do you not also reference the 400 year rule here?

What if there were more rules? ("only in evenly numbered decades, unless the decade is 1") Would you include them all in the description?

I still disagree - we have a decision tree, where the next rule ("unless") leads to an exemption of the current rule.

So

rule 1 fails => common year: only one rule to check, so only one rule in the description

rule 1 succeeds => possibly a leap year, we need to check rule 2

rule 2 fails: no exemption, so the result of rule 1 holds => leap year, no need to check further roles, so rule 1 and rule 2 in the description

rule 2 succeeds => exemption, possibly a common year, we need to check rule 3

rule 3 fails: no exemption, the result of rule 2 holds => common year, I mention the two really relevant rules (divisibility by 4 is obvious from mathematics if the number is divisible by 100)

rule 3 succeeds: exemption =>leap year only the last rule is really relevant (divisibility by 4 and by 100 is (again) obvious from mathematics.

I really appreciate how you force me to rethink the issue :-)

Thanks, that decision tree was very helpful for me to be able to see the way you're approaching the problem, and I can now see how the test descriptions you've used make sense in that case.

I also appreciate the effort you've put into this and hope my differing opinions have not discouraged you.

No worries - its good to make well thought decisions - especially when aiming at relatively inexperienced people - and in the end many future tests will draw from the templates in x-common.
How do I proceed from now on? I intend to squash all my commits, thereby amending the commit description. Should I wait on @stevejb71 ?

Do the squash and commit message changes and then @stevejb71 can review the "final" version.

@guntbert from this perspect, you will want to do a squash locally, and then force push your branch, which will set this up like you want it to be.

This now looks fine to me.

Insti · 2016-12-06T18:04:33Z

exercises/leap/canonical-data.json

-      {
-         "description": "even standard year in twentieth century",
-         "input": 1998,
+         "description": "year not divisible by 4: not a leap year",


Agreeing with Wikipedia: "A year that is not a leap year is called a common year." is a good idea.
"not a leap year" is a little too self referential.

Insti · 2016-12-06T18:05:29Z

exercises/leap/canonical-data.json

      },
      {
-         "description": "standard year in eighteenth century",
-         "input": 1800,
+         "description": "year divisible by 100, not divisible by 400: not a leap year",


The "not divisible by 400" is not relevant here.
What is important: "this year is divisible by 4 and also by 100."

I disagree: every year divisible by 100 is divisible by 4 too, so to me the important part is that in this case rule 3 does not apply.

But we do not need to implement rule 3 at all for this test to pass.

~~Perhaps switching the order of the 3rd and 4th tests would be better.~~

Edit: No, it's not. Test results should be false, true, false, true

Insti · 2016-12-06T18:07:19Z

@guntbert the 'slug' is the name of the exercise, in this case leap I have updated the title for you.

kotp · 2016-12-06T18:13:38Z

Updating the title won't update the commit though. You can amend the commit and do a force push on the branch, it will be OK.

When you make the changes, if you also squash the commits, that might be a good time to make that change.

guntbert · 2016-12-06T18:23:03Z

@kotp thanks for the heads up - does this repo allow squash commits from the webUI or should I squash on the command line?

kotp · 2016-12-06T19:53:28Z

Yes, to both questions. Either way you are comfortable with.

kytrinyx · 2016-12-07T23:54:23Z

sorry I have no idea what the "slug of the exercise" might be.

Sorry about that @guntbert, it's the url-friendly identifier that each exercise has. It's the same name that we would use in the config.json for it, and the name of the directory containing the metadata files. In this case, leap.

When looking through a few thousand commits, the subject line of the commit message Rewrite the test cases and their descriptions doesn't say which exercise it is referring to, which makes it harder to find things later.

- only test cases remain that are necessary - they have a clear description to help providing good names for tests

guntbert · 2016-12-08T11:08:50Z

I guess I am finished with this one now - thank you all for being clear, patient and attentive for details.

A big praise for the quick responses I got from the team - those made me feel welcome and supported.

guntbert · 2016-12-08T11:12:15Z

@kytrinyx thank you for the explanation - in hindsight it's obvious :-)

kotp · 2016-12-08T14:26:25Z

Thanks @guntbert and @stevejb71 and @Insti and of course Katrina!

the filepath needed used by example_gen.go was changed in exercism#224 the test cases were changed in this commit: exercism/problem-specifications#463

Adds the changes made to the the canonical test data in exercism/problem-specifications#463 and stores the test data version.

stevejb71 suggested changes Dec 6, 2016

View reviewed changes

Insti changed the title ~~Rewrite the test cases and their descriptions~~ leap: Rewrite the test cases and their descriptions Dec 6, 2016

Insti reviewed Dec 6, 2016

View reviewed changes

leap: Rewrite the test cases and their descriptions

be6fa53

- only test cases remain that are necessary - they have a clear description to help providing good names for tests

guntbert force-pushed the issue462 branch from 04a181e to be6fa53 Compare December 8, 2016 10:57

Insti approved these changes Dec 8, 2016

View reviewed changes

kotp merged commit 8b8666e into exercism:master Dec 8, 2016

guntbert deleted the issue462 branch December 8, 2016 15:08

This was referenced Jan 16, 2017

leap: update example_gen.go & test cases exercism/go#444

Merged

Generating tests references most recent commit, not relevant commit exercism/go#445

Closed

behrtam added a commit to behrtam/python that referenced this pull request Mar 22, 2017

leap: Update test cases

b128171

Adds the changes made to the the canonical test data in exercism/problem-specifications#463 and stores the test data version.

behrtam mentioned this pull request Mar 22, 2017

leap: Update test cases exercism/python#441

Merged

behrtam added a commit to exercism/python that referenced this pull request Mar 28, 2017

leap: Update test cases (#441)

82cd18b

Adds the changes made to the the canonical test data in exercism/problem-specifications#463 and stores the test data version.

petertseng mentioned this pull request May 9, 2017

leap 1.0.0.2: explain criteria in descriptions, put in progress order exercism/haskell#532

Merged

petertseng mentioned this pull request Aug 1, 2018

leap: Test case is not suitable to get desired solution. #1281

Closed

Uh oh!

leap: Rewrite the test cases and their descriptions #463

leap: Rewrite the test cases and their descriptions #463

Uh oh!

Conversation

guntbert commented Dec 5, 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!

kytrinyx commented Dec 6, 2016

Uh oh!

guntbert commented Dec 6, 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!

guntbert Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Insti Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Dec 6, 2016

Uh oh!

kotp commented Dec 6, 2016

Uh oh!

guntbert commented Dec 6, 2016

Uh oh!

kotp commented Dec 6, 2016

Uh oh!

kytrinyx commented Dec 7, 2016

Uh oh!

guntbert commented Dec 8, 2016

Uh oh!

guntbert commented Dec 8, 2016

Uh oh!

kotp commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

guntbert Dec 7, 2016 •

edited

Loading

Insti Dec 6, 2016 •

edited

Loading

kotp commented Dec 8, 2016 •

edited

Loading