Is the custom-set test set too long? #10

IanWhitney · 2016-04-18T01:42:03Z

I'm working on re-working the Rust implementation so that it follows the common test suite. I'm currently at 32 tests and I might be half-way through.

This seems long. It seems to be that a lot of these tests are repetitive. Or maybe they are trying to catch weird corner cases that simply aren't occurring to me.

Some examples

Testing non-empty sets, why two tests? https://github.com/exercism/x-common/blob/master/custom-set.json#L134
Testing element presence, why four tests? https://github.com/exercism/x-common/blob/master/custom-set.json#L176
Subset testing appears to have lots of repetition https://github.com/exercism/x-common/blob/master/custom-set.json#L235
Eight tests of intersection: https://github.com/exercism/x-common/blob/master/custom-set.json#L289
And eight of Union: https://github.com/exercism/x-common/blob/master/custom-set.json#L342

I'm not a Set Theory expert, so maybe these tests are catching useful bugs, but it does seem to me that there's a lot that could be trimmed.

Thoughts?

IanWhitney · 2016-04-18T01:43:37Z

In most cases as soon as I get a function's first or second test to pass, all of the others pass without problem. This suggests to me that those tests aren't catching corner cases. Though they might be catching corner cases that I didn't happen to hit.

IanWhitney · 2016-04-18T02:04:11Z

Also, I'm wondering if the whole mixed_types section can just be dropped. With languages that enforce that collections contain the same types (like Rust, Haskell, etc), I'm not even sure how to go about implementing this.

That's not to say it's not implementable, just that I have no idea how. It might be beyond the scope of the problem.

kytrinyx · 2016-04-18T10:50:46Z

/cc @exercism/track-maintainers

ryanplusplus · 2016-04-18T11:12:54Z

This is something I've noticed with a lot of the common tests. Recently I went through the Clock tests and they also seemed to be a little bit overdone. I think that some of the tests are aimed at producing NASA-grade implementations instead of producing something that can be critiqued for style.

On the other hand, the tests only need to be written once and I've never felt that a long list of tests burdened me when doing an exercise, so maybe it's better to have these bullet proof tests.

verdammelt · 2016-04-18T11:20:31Z

Looks like the first two groups of tests you point to are in the common 0, 1, many pattern. I don't see that is being too redundant given that we are talking about a container. I'm not seeing the duplication in the subset tests and the intersection tests look "OK" in my skimming of them.

If I were TDD-ing a set implementation I might not have all these tests myself (e.g. the intersection test that tests against empty as the first and second parameter) but I don't think they do much harm.

IanWhitney · 2016-04-19T01:15:25Z

@ryanplusplus, I'm less concerned about the work it takes maintainers to implement the exercise and more with the effect on students. There are two problems I see with big test suites, especially ones with redundant tests.

It makes the problem seem huge. The Rust test suite update I'm working on is currently 400 LOC, and I have about 2 dozen test left that I could implement. If I'm a student coming to that test suite, the problem space is going to seem massive.
Repetitive tests offer no educational value. Let's take the [8 union tests] as an example. They test

two empty sets
An empty set and a set with one element
An empty set and a set with 3 elements
An set with 2 elements and an empty set
2 sets with the same elements
A set with 2 elements and a set with 1 element
2 sets with 2 elements
Sets with some shared, some different elements

Assuming that a student is working their way through the tests, getting 1 to pass at a time, at which point do all of these tests pass? I'm guessing by test 5, when we check for duplicate elements. If so, what does a student learn from the remaining 3 tests?

I think nearly all of these test collections could be consolidated into 3 tests each. Union, Intersect, Diff and Symmetric Diff could each have 3 tests:

2 empty sets
1 empty, 1 non-empty
2 non-empty sets with some shared elements

IanWhitney · 2016-04-19T01:19:28Z

A good indication I've found with test duplication is that I can't find a way to make test names unique. If I see a set of tests named:

union_of_two_non_empty_sets_contains_all_unique_elements_one
union_of_two_non_empty_sets_contains_all_unique_elements_two
union_of_two_non_empty_sets_contains_all_unique_elements_three

Then I figure either I'm unimaginative (possible) or the tests cover the same ground.

petertseng · 2016-04-19T03:56:13Z

I suppose I don't have much to say about this specific issue since I haven't worked on the custom-set issue in any language. What I can see, however, is something that seems useful in general. If someone make a test json file, it will probably be useful to explain what some of the corner cases are testing (what kinds of solutions might not pass this test? why?). Or at least in the commit message.

That should probably go in a separate discussion though. ~~I'll see if I can find the right place to put it, or remind me in some days.~~ Created.

kytrinyx · 2016-04-19T19:14:33Z

I would like to reduce duplication/unnecessary test cases in the tests.

(Side note: With the clock tests I added more tests, because people were submitting solutions that had bugs that the test suite (in Go) wasn't catching. This was happening often enough that I thought more tests would be helpful. It's an interesting problem in Go; I don't know what it's like in other languages).

I'm addressing two things in this commit. Bringing Tests in line with the Standard ==== Our suite of tests for Custom Set did not follow the [standard json file in x-common](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json) They tests have different function names, don't cover symmetric_difference and introduced some functions (`is_superset`) that aren't in the standard test file. I have not implemented the entire standard test suite as it covers sets of [mixed types](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json#L475) which I don't think is even possible in Rust. Vectors have to be homogeneous. Tuples can mix types, but can't change size. Mixed type sets may be possible, but implementing that seems way beyond the scope of this exercise. I think [this suite of tests is too long](exercism/discussions#10). But until I get a PR in to shorten the standard test suite, I figured I might as well follow it. Updating the API ==== I had a couple of problems with the current approach. First, the tests hide the actual API in a helper method. This was probably done to reduce duplication, but I find that it obscures the function under test. So I removed those. The tests also pushed towards a bunch of implementation that I found unnecessary, specifically the whole [Iter implementation](https://github.com/exercism/xrust/blob/04b665e989b578c4e46ff100b1b0a00d5df9bb0d/exercises/custom-set/example.rs#L118). There might be ways to get the current test suite passing without implementing Iter, but I got pretty stuck. My new implementation does not require this implementation, since equality comparisons are now handled by implementing PartialEq.

IanWhitney · 2016-04-20T12:54:46Z

I'll put together a shorter test suite and put it up for a PR. See what people say.

Discussed here: exercism/discussions#10 I'm trying to improve custom_set in two ways: 1. Reducing the test suite, to remove redundancy or uninteresting implementations. 2. Changing test order to improve flow Reducing the test suite ---- The previous test suite contained 74 tests, which is a lot. I haven't checked all the exercises, but it's the biggest test suite that I've come across. If all of those tests provided value (exposing corner cases, improving implementations, etc.) then that's fine. But I found there to be a lot of duplicate tests. With the subset/union/etc tests, a student's implementation is usually done by the 2nd or 3rd test, so the remaining tests didn't provide any additional value. So I have removed the tests that seemed redundant. The tests also expected methods like `size`, `delete` or `is_empty`. I have removed those because - They aren't vital to the behavior of a Set - They are usually implemented as an alias - They aren't used by the set operations (diff/subset/etc) Changing test order ---- The previous test suite started with `equal`. I found that this requires students to implement two things: - Creating a new element - Comparing two collections of elements I have chosen to start the tests with `contains`, since that only requires one set. And, helpfully, when the student implements `add` and `equal`, they can leverage their already-existing `contains` function.

kytrinyx · 2016-04-21T13:15:45Z

Union, Intersect, Diff and Symmetric Diff could each have 3 tests [...]

@IanWhitney what about two non-empty non-intersecting sets?

IanWhitney · 2016-04-21T13:44:38Z

My claim that they should have 3 was wrong. I think each ended up with 4 tests. Did I cover the case you're asking about? https://github.com/IanWhitney/x-common/blob/reduce_custom_set_tests/custom-set.json#L204

IanWhitney · 2016-04-21T13:47:10Z

{1,2} ∩ {3,4} = {} is what you mean, right? Sorry, early morning & my set math brain is not working. That should be covered with this test

masters3d · 2016-04-21T17:54:39Z

Multiple tests does not bother me al long as the test suite is able to group them into a "single" function.
https://github.com/exercism/xswift/blob/master/exercises/custom-set/CustomSetTest.swift
If my test suit forced me to break out every case into its own function then that would be burdensome.

kytrinyx · 2016-04-25T17:32:17Z

@IanWhitney yeah, that's what I was thinking of.

I'm addressing two things in this commit. Bringing Tests in line with the Standard ==== Our suite of tests for Custom Set did not follow the [standard json file in x-common](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json) They tests have different function names, don't cover symmetric_difference and introduced some functions (`is_superset`) that aren't in the standard test file. I have not implemented the entire standard test suite as it covers sets of [mixed types](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json#L475) which I don't think is even possible in Rust. Vectors have to be homogeneous. Tuples can mix types, but can't change size. Mixed type sets may be possible, but implementing that seems way beyond the scope of this exercise. I think [this suite of tests is too long](exercism/discussions#10). But until I get a PR in to shorten the standard test suite, I figured I might as well follow it. Updating the API ==== I had a couple of problems with the current approach. First, the tests hide the actual API in a helper method. This was probably done to reduce duplication, but I find that it obscures the function under test. So I removed those. The tests also pushed towards a bunch of implementation that I found unnecessary, specifically the whole [Iter implementation](https://github.com/exercism/xrust/blob/04b665e989b578c4e46ff100b1b0a00d5df9bb0d/exercises/custom-set/example.rs#L118). There might be ways to get the current test suite passing without implementing Iter, but I got pretty stuck. My new implementation does not require this implementation, since equality comparisons are now handled by implementing PartialEq.

I'm addressing two things in this commit. Bringing Tests in line with the Standard ==== Our suite of tests for Custom Set did not follow the [standard json file in x-common](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json) They tests have different function names, don't cover symmetric_difference and introduced some functions (`is_superset`) that aren't in the standard test file. I have not implemented the entire standard test suite as it covers sets of [mixed types](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json#L475) which I don't think is even possible in Rust. Vectors have to be homogeneous. Tuples can mix types, but can't change size. Mixed type sets may be possible, but implementing that seems way beyond the scope of this exercise. I think [this suite of tests is too long](exercism/discussions#10). But until I get a PR in to shorten the standard test suite, I figured I might as well follow it. Updating the API ==== I had a couple of problems with the current approach. First, the tests hide the actual API in a helper method. This was probably done to reduce duplication, but I find that it obscures the function under test. So I removed those. The tests also pushed towards a bunch of implementation that I found unnecessary, specifically the whole [Iter implementation](https://github.com/exercism/xrust/blob/04b665e989b578c4e46ff100b1b0a00d5df9bb0d/exercises/custom-set/example.rs#L118). There might be ways to get the current test suite passing without implementing Iter, but I got pretty stuck. My new implementation does not require this implementation, since equality comparisons are now handled by via PartialEq.

Discussed here: exercism/discussions#10 I'm trying to improve custom_set in two ways: 1. Reducing the test suite, to remove redundancy or uninteresting implementations. 2. Changing test order to improve flow Reducing the test suite ---- The previous test suite contained 74 tests, which is a lot. I haven't checked all the exercises, but it's the biggest test suite that I've come across. If all of those tests provided value (exposing corner cases, improving implementations, etc.) then that's fine. But I found there to be a lot of duplicate tests. With the subset/union/etc tests, a student's implementation is usually done by the 2nd or 3rd test, so the remaining tests didn't provide any additional value. So I have removed the tests that seemed redundant. The tests also expected methods like `size`, `delete`. I have removed those because - They aren't vital to the behavior of a Set - They are usually implemented as an alias - They aren't used by the set operations (diff/subset/etc) The new test suite has 37 tests. A 50% reduction. Changing test order ---- The previous test suite started with `equal`. I found that this requires students to implement two things: - Creating a new element - Comparing two collections of elements I have chosen to start the tests with `empty`, followed by `contains`, since those can be passed quickly. When the student implement the later functions they can leverage their already-existing functions.

petertseng mentioned this issue Apr 19, 2016

Add instructions on how to extract canonical data to the Contributing Guide exercism/problem-specifications#230

Closed

IanWhitney mentioned this issue Apr 20, 2016

Update Custom Set tests to follow standard, update API exercism/rust#114

Merged

IanWhitney mentioned this issue Apr 20, 2016

Shrink & clean up custom_set.json exercism/problem-specifications#232

Merged

IanWhitney closed this as completed Jun 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Is the custom-set test set too long? #10

Is the custom-set test set too long? #10

IanWhitney commented Apr 18, 2016

IanWhitney commented Apr 18, 2016

IanWhitney commented Apr 18, 2016

kytrinyx commented Apr 18, 2016

ryanplusplus commented Apr 18, 2016

verdammelt commented Apr 18, 2016

IanWhitney commented Apr 19, 2016

IanWhitney commented Apr 19, 2016

petertseng commented Apr 19, 2016 •

edited

kytrinyx commented Apr 19, 2016

IanWhitney commented Apr 20, 2016

kytrinyx commented Apr 21, 2016

IanWhitney commented Apr 21, 2016

IanWhitney commented Apr 21, 2016 •

edited

masters3d commented Apr 21, 2016

kytrinyx commented Apr 25, 2016

Is the custom-set test set too long? #10

Is the custom-set test set too long? #10

Comments

IanWhitney commented Apr 18, 2016

IanWhitney commented Apr 18, 2016

IanWhitney commented Apr 18, 2016

kytrinyx commented Apr 18, 2016

ryanplusplus commented Apr 18, 2016

verdammelt commented Apr 18, 2016

IanWhitney commented Apr 19, 2016

IanWhitney commented Apr 19, 2016

petertseng commented Apr 19, 2016 • edited

kytrinyx commented Apr 19, 2016

IanWhitney commented Apr 20, 2016

kytrinyx commented Apr 21, 2016

IanWhitney commented Apr 21, 2016

IanWhitney commented Apr 21, 2016 • edited

masters3d commented Apr 21, 2016

kytrinyx commented Apr 25, 2016

petertseng commented Apr 19, 2016 •

edited

IanWhitney commented Apr 21, 2016 •

edited