book-store: Add test checking that solutions can mix groups of 4 and 5

[sswingle]

Many of the "brute force" solutions I see do something like run a greedy solver multiple times with different maximum group sizes. This allows them to solve test cases that check for finding 2 groups of 4 instead of a group of 5 and a group of 3.

However, this wouldn't solve cases where you need to use both groups of 5 and groups of 4. For example, consider books = [1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2, 3]. The cheapest price possible is achieved by splitting it into 1 group of 5 and 2 groups of 4, with cost = 800*(1*5*0.75 + 2*4*0.8) = 8120. Solutions like those described above will typically split it into 2 groups of 5 and 1 group of 3, with a cost of 800*(2*5*0.75 + 1*3*0.9) = 8160. An example is the compact brute force solution @yawpitch shows here: https://exercism.io/tracks/python/exercises/book-store/solutions/c58f3dcf78314e95820c717a30f787c9 (your recursive solution returns the correct value of 8120, of course).

This PR adds the test case described above.

[cmccandless]

A couple of things:

1. This exercise test suite is generated. The test file itself should not be directly changed. This is (one reason) why the CI build failed; it detected that what is generated and what is checked in are not equal.
2. New tests should generally be added to the canonical data. Once changes have been made there, the test suite can be regenerated to include the new test and, if necessary, the example solution can be updated to pass the new test(s).

[yawpitch]

@sswingle this is a very good catch ... I didn't try too hard to come up with a target for which the two approaches disagreed. This one really is worth adding to the canonical data file that @cmccandless mentioned, so please open an issue or PR there. Unfortunately there's a temporary hold on merges for that repo, but it'll be good to have it documented for when that lifts.

[sswingle]

@cmccandless @yawpitch thanks for taking the time to look at this and point me in the right direction. I'll make a PR for the canonical data change. Looks like the other reason the CI build failed was that the example solution for book store only correctly works for input that's sorted (i.e. my test should be [1, 1, 1, 2, 2, 2, 3, 3, 3, 4, 4, 5, 5]).

When the hold on merges on the other repo lifts, what happens when I merge the PR there? Does this PR need to go through at the same time so that builds work? What about other language tracks?

[cmccandless]

When the hold on merges on the other repo lifts, what happens when I merge the PR there? Does this PR need to go through at the same time so that builds work? What about other language tracks?

This PR will need to merge shortly after the update to canonical-data, yes. Other language tracks will likely adopt the new canonical version as well in their own time (not every track has failing CI when tests are out-of-date).

The example solution here will also need to be updated to pass the new test. My advice there would actually be to sort the input inside the example solution, not in the test, as this makes the solution more robust.

[sswingle]

I've created a PR over on problem-specifications (exercism/problem-specifications#1620)

On this one I've edited the example solution to sort inputs. This should pass tests and be read to merge when the problem-specs one merges.

[yawpitch]

I've created a PR over on problem-specifications (exercism/problem-specifications#1620)

On this one I've edited the example solution to sort inputs. This should pass tests and be read to merge when the problem-specs one merges.

To be accurate this PR is not in a state where it could be merged when exercism/problem-specifications#1620 is merged; because the tests are automatically generated from the canonical data the template would need updating, not the book_store_test.py file directly.

Also there's a subtle issue with the suggested change to the example.py file... should a Python function ever sort the list provided as an argument by the user for them?

>>> books = [5, 5, 4, 4, 3, 3, 2, 2, 2, 1, 1, 1]
>>> print(total(books))
8120
>>> print(books)
[1, 1, 1, 2, 2, 2, 3, 3, 3, 4, 4, 5, 5]

[cmccandless]

should a Python function ever sort the list provided as an argument by the user for them?

No, it shouldn't. It should create a copy, and sort that.

[sswingle]

To be accurate this PR is not in a state where it could be merged when exercism/problem-specifications#1620 is merged; because the tests are automatically generated from the canonical data the template would need updating, not the book_store_test.py file directly.

I'm not changing the formatting of any tests, so the template doesn't need to be changed - my changes to book_store_test.py are what was generated by the template, and I assume they have to be checked in as part of a PR since book_store_test is checked in.

Also there's a subtle issue with the suggested change to the example.py file... should a Python function ever sort the list provided as an argument by the user for them?

Ah, of course not...

[sswingle]

I've now updated to add the second test I added in the problem-specifications PR (for shuffled book data). It was generated from the template, so should reflect that PR exactly.

[yawpitch]

I'm not changing the formatting of any tests, so the template doesn't need to be changed - my changes to book_store_test.py are what was generated by the template, and I assume they have to be checked in as part of a PR since book_store_test is checked in.

You're correct, misread and thought you were directly editing the test file. The usual process is that we pull changes to the problem-specifications, run the test builder, then do a PR that just reflects a sync with the upstream repo.

[sswingle]

Yeah that makes sense. Thanks for the help! I guess now we just wait on the other repo.

[stale]

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.