kindergarten-garden: Inconsistency between description and tests

[behrtam]

@dzhoshkun wrote exercism/python#423

The problem description says there are 12 children in the class (named Alice through Larry), however the test suite includes a test with other names. This is a bit confusing. I think it would be better to keep the README and the test suite consistent.

At least Python (test) and Ruby (test) use those names not mentioned in the description. I don't see it as an inconsistency but I thought it might be better discussed here than inside a specific language track.

[petertseng]

And if #770 is merged in its current state, the tests using other names will be in the canonical data.

That will mean that the test with other names will be official.

Therefore, unless we wish to remove those tests, it seems that I should recommend that the way to resolve this issue would be to change the README to indicate that there can be other kids.

[petertseng]

This is how the exercise has always been since exercism/exercism@2d50787 (with the test in question called SurpriseTest, implying that its absence in the description indicates that it is supposed to be a surprise requirement).

Despite this, ever since exercism/go#189 I have preferred that the description describe the requirements accurately. Granted, the omission might not force as much extra work, but it would probably still force some nonzero amount.

I think it would be good to change the README.

[m-sameer]

Can you tell me where other names are?

[Insti]

The problem description lists some names: https://github.com/exercism/problem-specifications/blob/master/exercises/kindergarten-garden/description.md

There are 12 children in the class:
Alice, Bob, Charlie, David,
Eve, Fred, Ginny, Harriet,
Ileana, Joseph, Kincaid, and Larry.

However the test data also lists some other names:
https://github.com/exercism/problem-specifications/blob/master/exercises/kindergarten-garden/canonical-data.json#L92

"students": ["Samantha", "Patricia", "Xander", "Roger"],

The conclusion is to update the description.md file to add something like:

"You may also need to report on other classes with different students."

[m-sameer]

Oh okay. I'll make a PR.

[m-sameer]

@Insti I think I should add a "Test Cases" column for describing the test cases or something like "Disclaimer: You may also need to report on other classes with different students.". What do you think?

[Insti]

It is better to keep pull requests to a single issue, and this issue is about the names of the students in the class.

The test cases are described in the canonical-data.json file and should be described as part of the test file for a particular language track. Tracks can also implement their own additional tests so we don't tend to explicitly specify them in the problem description.

But if you think it would be an improvement, you can submit a PR which makes your proposed changes and we would then have something concrete to discuss.

[m-sameer]

I created a PR. Please merge it.
Or tell me if there's something else I have to do.

[NobbZ]

After the PR has been made I've taken a closer look into the description and the canonical data…

To be honest, data and description do not belong together… The data is for a completely other exercise than that that is in the description.

Either we have 12 students in the class, named in the README and use them throughout the exercise, or we generate a complete new set of students everytime.

This is pretty much in the same vein as the sum of multiples discussion we had a couple of months (years?) ago, were we had an arbitrary default list of [3, 5]. Now we have a similar arbitrary default of the 12 names described in the README.

The testdata seem to assume that we generate the default names if none are given, but use a different set of names if provided.

There is no proper explanation of how to handle those names. Shall we sort their pots in alphabetical order? Ascending? Descending? In the same order as their names were given?

So my proposal is either to remove those “surprise test” completely as it is contrary to the exercise description or to remove the list of 12 names from said description and rephrase in a way that tells the exercism student what he has to do.

• Create a class from names
• Read the diagrams (left side is first name from the list of names, right side the last name)
• Map each set of 4 pots to their owners

Also the given example diagrams should be made in a way, that we do not have the 12 in there… A class can have 1 pupil, 10 pupils, 12 or even 50 (well, I do not want to go to that school ;)) since we create it from the list of names.

[m-sameer]

I think creating extra more students won't contribute to our problem-solving skills. @NobbZ What do you think?

[NobbZ]

Well, the problem is, that the README says we have 12 students, but the tests start out with only a 2 by 2 diagram for the “default class”. The README does not explain how to treat that. The current README says, we have 12 students and each student has 4 pots.

So according to the README that testcase is simply invalid.

But since the testdata is already driving to a solution which is able to handle arbitrary sized classes, the number of students shouldn't matter at all anymore (if following the testdata).

If though we want to follow the current README, we will not be allowed anymore to test partial classes.

[m-sameer]

The solution would be to edit README.

[m-sameer]

Can someone tell me what I have to do now?

[NobbZ]

@mhdsmr2 probably waiting…

As I am writing this, there are two 👍 reactions on my post, but none of both have said anything, about which of the proposals they like. So I do read their 👍 as “we agree that something needs to change, but we are not sure how exactly”.

But to be honest, I'm not sure whom we could ask here… So just lets ping @kytrinyx, either she will have an opinion or she will know whom to call for help ;)

[Insti]

we have 12 students in the class, named in the README and use them throughout the exercise,

Do this.

This still allows for testing short rows.

Supporting using other names is probably a better design, but is not necessary for the specified problem and makes it slightly more complicated.

Remove or modify any tests that are testing things not already specified in the description.

[NobbZ]

Remove or modify any tests that are testing things not already specified in the description.

Thats my first thought as well.

This still allows for testing short rows.

How?

My first thought were, to only use the first x students when I get x 2 by 2 patches, but then we should clarify this in the README.

[Insti]

This still allows for testing short rows.

How?

I would do it the way you suggested, but I guess that's another case of "maybe better design but unnecessary problem modification"

The input should always be 2x24 characters long and contain only valid plants.
Care should be taken to ensure that the plants of the "interesting" student used in the test are easily distinguishable by the solver of the exercise. For example, by using the same character for everybody else's plants.

[m-sameer]

So, I have to change every test case to be of size 2x24 and remove all the other students which are not mentioned in the description. right?

[Insti]

If you want to and think that is a good idea.
A pull request is usually a good for encouraging discussion.

remove all the other students which are not mentioned in the description.

Or change the names, it depends on what the tests are testing.

[m-sameer]

None of the test cases' description is testing for the solution to be able to support names of different names other than the README ones (if "non-alphabetical student list" doesn't mean different names of the students and only mean a unsorted list of students). So the only thing I have to change is names of the students.

[Insti]

The description says:

The children are assigned to cups in alphabetical order.

The tests have:

"description": "non-alphabetical student list",

a non-alphabetical student list is incompatible with the description, so that entire test section should probably be removed.

[m-sameer]

Don't you think removing that section will left us with really few test cases?

[Insti]

Don't you think removing that section will left us with really few test cases?

What is the right number of test cases?

[m-sameer]

There are four sets and the first one have three test cases, the second one has two test cases, the third and the fourth one have four test cases. If we remove the fourth one(in which the names of students is different then the description. I think we should just change the names in the last set, what about that?

[Insti]

But what are the tests testing?

The last set seems to be testing different names in non-alphabetical order, but that is not specified in the description and is not really related to the main part of the problem which is allocating plants to the listed and pre-ordered students.

[m-sameer]

Yeah, so we should just remove the last set of test cases. Should I commit that to my PR repo?

[Insti]

Yes. Put the Readme back how it was, and remove the tests.

[m-sameer]

I did the changes in the PR.

[kytrinyx]

I'm late to the party. I don't have strong opinions—I think @Insti and @NobbZ are spot on for their suggestions for simplifying and specifying. ✨

[m-sameer]

@kytrinyx I made a PR which does what @NobbZ and @Insti said(remove the test set which uses names of students other than mentioned in the README), have a look.