crypto-square: Improve test data

[nholden]

This cleans up a few things about crypto-square's canonical data that I thought were worth addressing before I generated new tests in the Ruby repo to address exercism/ruby#422.

• Removes unnecessary sub-groupings of test cases (related discussion)
• Moves the blank string test case to the top since it feels like the simplest one to get passing using TDD
• Adds a new test case

[nholden]

This is the new test case, which I borrowed from the source of the problem. I think it's a useful bridge between the previous test case, which tests a perfect rectangle, and the next test case, which like this one tests an imperfect rectangle but is quite a bit longer.

[nholden]

@Insti, since we discussed some of this in #922, would you mind taking a look when you have a chance?

[Insti]

Very nice. 👍

[Insti]

I'm not sure the version needs to go to 4.0, maybe 3.1 is enough?
Does the removal of test groupings makes a semver relevant difference?

See also: #936 (review)

[NobbZ]

I now took a look into the version numbering documentation…

This sentence:

MAJOR changes should be expected to break even well-behaved test generators.

Makes this changes (as well as the other two I've requested changes in) worth a major bump.

But what exactly is “well behaved”? To be honest… I do expect about every change, that goes beyond renaming or adding/removing a test to break the generator.

But especially adding and removing tests, are often responsible for a major bump as well, since they can change the exercises path, the way to solve it, even the resulting program a lot…

So I conclude: just omit minor and patch, do only major in the future…

But that is probably better discussed elsewere…

PS: Even though I think a minor is sufficient here, the removal of the additional cases layer probably breaks generators, so major seems valid :(

[nholden]

Thanks for the quick review!

I'm not sure the version needs to go to 4.0, maybe 3.1 is enough?
Does the removal of test groupings makes a semver relevant difference?

Good question. After I read the section in the README about minor version changes, I thought your point seemed like a good one and revised the original commit to make it a minor version change.

However, I think @NobbZ has a good point as well:

But what exactly is “well behaved”? To be honest… I do expect about every change, that goes beyond renaming or adding/removing a test to break the generator.

I'm not familiar with how generators work across all languages, so maybe with the removal of test groupings, it would be safer to make this a major version change.

Does anyone have strong feelings one way or the other?

[NobbZ]

Even worse…

[MINOR when] Regrouping/"Renesting" test cases without changing test case ordering.

But I expect this to break the generators, since the cases aren't anymore were the generator expects them…

[nholden]

[MINOR when] Regrouping/"Renesting" test cases without changing test case ordering.

But I expect this to break the generators, since the cases aren't anymore were the generator expects them…

I think the bit you quoted makes it clear that this should be a minor version bump given the current policies. If regrouping/renesting test cases does break generators, I think it's worth re-opening #673 or creating a new issue.

[ErikSchierboom]

Looks good!