[High Scores]: Improve Test Cases / Exercise for Mutability Discussions

[12rambau]

I just did this exercise in php and python and I think that the Python version would require some improvement. I would like to discuss if it makes sense and eventually contribute via PR.

explanations

When you jump in the exercise, the explanation are very small. the user faces 4 functions and without looking at the test I would have never understood what the last were about (https://github.com/exercism/python/blob/main/exercises/practice/high-scores/high_scores.py).

def latest(scores):
    pass

def personal_best(scores):
    pass

def personal_top_three(scores):
    pass

def latest_after_top_three(scores):
    pass

def scores_after_top_three(scores):
    pass

In some exercises there is a complete description of the tasks and in other the function objective is described in the docstring (or bot). Would it make sens to add this for this exercise?

latest_after_top_three and scores_after_top_three

I think these 2 functions are perfectly useless in Python. I actually validated the tests (

python/exercises/practice/high-scores/high_scores_test.py

Line 50 in 79515ff

def test_latest_score_after_personal_top_scores(self):

) with the following:

def latest_after_top_three(scores):
    return latest(scores)

def scores_after_top_three(scores):
    return scores

From what I did in php I think that these function reflect the C like behavior where passing scores by reference in the latest or personal-top-three function could have altered the initial scores list.

So instead of having these functions, I think it would make more sense to test the scores variable after running personnal_top_three.

let me know what you think and more importantly let me know if I'lm wrong.

PS: As mentioned at the top of the issue I'm happy to contribute

[BethanyG]

Hi @12rambau 👋🏽

Thanks for filing this issue. I think you bring up some valid points, some of them addressable, and some of them not addressable. Bear with me, there is a bit to unpack here:

When you jump in the exercise, the explanation are very small. the user faces 4 functions and without looking at the test I would have never understood what the last were about.

This is "by design". For Practice exercises, we give very minimal stubs. The 'ethos' behind this is that it is "test driven development". You should be looking at the tests to get a feel for what is expected.

In some exercises there is a complete description of the tasks and in other the function objective is described in the docstring (or bot). Would it make sense to add this for this exercise?

You are most likely seeing a difference between concept exercises (these are the ones linked from the main "nodes" of the site syllabus tree and practice exercises (everything else).

Concept exercises are meant to zero in on one set of closely related Python Language concepts. They have fairly particular solutions, and very detailed instructions and stub files. The test files for these exercises are hidden on the site, and are not intended to be referred to or read by the student prior to solving the problem -- hence the detailed stubs and task sets.

Practice exercises are much looser, and focus more on a problem that practices multiple techniques in the language and/or algorithms or design decisions. For those problems, implementation is less directed, problem descriptions are less detailed (and are mostly shared across tracks) and the test files are intended to be looked at as part of the problem-solving process.

Due to these differences in intent/process, we have not added detailed stubs nor type hinting to practice exercise files, and are not likely to do so. We don't want to over-specify implementation, and we expect that students will (mostly) engage with mentors to talk through different design decisions and approaches.

I think these 2 functions are perfectly useless in Python. I actually validated the tests.

I agree with you. As this exercise is currently implemented in Python (functions only, no class required, no saved state for scores required), these two functions make very little sense, and it would be easiest if we skipped both the functions and the associated tests all together, and left the discussion of mutability to a mentoring session.

Python track tests for practice exercises are atuo-generated based on specifications from the problem-specifications repo. Descriptions and data for these exercises are shared across tracks, and so are declarative and rather non-specific. Tracks can (and do) deviate -- but they have to do so through instruction and test appends -- OR -- the proposed change has to be accepted by 3 maintainers in problem-specifications and rolled out cross-track.

So this isn't a matter of "simply" editing the test files or test cases -- for Python, we will need to edit:

1. The test generation template,
2. Add skips (include=false) for the additional (new) tests in the test spec.,
3. Write addendum tests (see an example here) for the particular scenarios,
4. Clarify what we've done by adding an addendum to the canonical instructions.

So I am not against changing things. In fact, I think this exercise might be better in its original version, where we asked students to make a class with various methods to track scores, and pointed them in a direction where mutability had to be dealt with. But any changes will require the work above.

So I see several "paths" to addressing this issue:

1. Leave the useless and rather irritating function stubs and tests there.
2. Omit the new test cases and the need for the useless functions. Leave mutability to a mentor discussion.
3. Skip the new test cases and write both a test case append and and instruction append to get at mutability issues.
4. Re-write the exercise to require a class, and better set up the mutability debate.

Happy to discuss all of this further. Also happy to have you PR the work. But at this time, my vote would be for options 1 or 2 for expediency, and then a longer discussion about re-working the exercise.

Hope that all makes sense -- and thank you for reading all this!

[12rambau]

woaw, that's a complex and very specific process. But that's great !

Thanks for enlightening me on the difference between Concept exercises and Practice exercises that's more clear now and I think forcing us to do some TDD is a good thing.

On my issue, and as the process require a lot more work than I expected, I'll see how much time I can dedicate to it but I'll be glad to see what's the worklfow to participate in exercism. I'll keep you updated this WE

[BethanyG]

On my issue, and as the process require a lot more work than I expected, I'll see how much time I can dedicate to it but I'll be glad to see what's the worklfow to participate in exercism.

Just to be clear: This complexity is for an existing practice exercise and amending or changing the exercise or the tests.
The process looks different for new exercises, and for concept exercises, or for small bug fixes or typos. So there are other places to contribute to exercism and to the Python track that have a much less complicated workflow. 😄  

Feel free to hit us up (@J08K and myself are the maintainers for Python) if you are looking for other, and less involved ways of contributing.

[BethanyG]

@12rambau -- Just checking in here. 😄

I went ahead and backed out the mutability test cases and regenerated the test files in PR #2808. This way, there is no need for effectively "empty" or "useless" functions in the exercise.

But I am going to leave this issue open in case you'd like to PR additional test cases for Python that would get at discussing mutability issues. Or if you would like to work on converting this exercise to one that uses a class.

Just let I or @J08K know -- we'd be happy to help you with test case appends, or any other questions you have.

[12rambau]

I think this exercise could be super useful for mutability, unfortunately, I'm swamped in webinars for work and preparation is so much time-consuming....

Thanks a lot for your PR and I agree we should leave it open, at some point I'll have time to breath and work a PR for this exercice!

[BethanyG]

@12rambau -- just a note here. Mutability has come up in another exercise discussion - this one around Clock. I think we've decided that rather than change Clock, we're going to bite the bullet and re-implement High Scores as a class-based exercise, setting up a mutability discussion. To that end, @IsaacG has stepped forward to take that on. I'm going to past the start of our discussion around that here, in case you'd like to chime in.

[BethanyG]

@IsaacG -- pasting my last comment from #3008 below:

---

Should discussion about updating High Scores to cover mutability be in this Issue or elsewhere?

Elsewhere. Maybe we do it in 2786, since that's the last filed issue against High Scores, and does discuss mutability. That way, the author of that issue will also get a ping letting him know you've picked it up - in case he wants to join in the discussion.

I think (for now) we close this issue -- and also the other Clock issue. We may want to revisit this exercise, but not right now. I feel like it's been mucked with enough for the time being.

How do you feel about simply testing if those functions mutate the data without introducing a class vs adding a class to the exercise?

Thats what the original class-based High Scores did, and you can see that in the tests.toml file. When it was re-written to be function-only, it ... got funky. See #2786 for what happened.

Adding a test to check if the functions mutate the data ought to be relatively simple and straight forward.

This may be a case where simply re-instituting the exercise as it was in 2018/2019 will do the trick, and then altering the test generation template to import the class instead of functions, and changing the tests.toml file to re-activate the tests cases. Since we ported repos for V3, I need to dig for that state. So there may not be any work beyond that for a base state.

But I think we should also take a look at the point you were making with Clock, and ask if that same point could be made here with a "ScoreKeeping" class. There is the question of mutating data - but there is also the questions of mutating state, and the issues and considerations around that. This might be a good opportunity to revisit what we introduce in the class concept exercise, where we have a class attribute and also instance attributes.

[BethanyG]

@IsaacG - The resurrected files from High Scores are here in a draft PR.

As noted, there is still work around getting the immutability cases marked true and modifying the JinJa2 template to support them. It should be fairly straighforward. After the template is modified, the test case file will need to be regenerated.

Let me know if you have questions or issues. Since I did have to do some work to make the JinJa2 template work, I'd prefer to get credit for that via the PR -- so I don't know if we want to merge and then have you do the modifications separately, or if you modify the PR directly by branching or forking.

[BethanyG]

here is the canonical data for the exercise.

[IsaacG]

1. I'm not clear why or how adding in a class would or should impact the mutability aspect of the tests. The idea that functions should (generally) not mutate input data is distinct from classes. This approach may highlight that class methods should not mutate class instance attributes as a side effect. However, I feel that the lesson around mutating function inputs is less obvious and, as such, more important to deliver.
2. I can poke at revamping the Jinja template to tweak the output data. What are you hoping to see for the generated test code? I don't think it makes sense for the solution to implement the *_after_* functions; the after describes operations performed by the test and not functionality which students should be implementing. I'm thinking we could either import and inherit the HighScore class and add <a>_after_<b> functions, which would call self.a() then return self.b(). Alternatively, the generated code could explicitly expand the <a>_after_<b> tests to call a() then test b().

Thoughts?

[BethanyG]

However, I feel that the lesson around mutating function inputs is less obvious and, as such, more important to deliver.

I remain skeptical that this particular concept can be taught through tests. It really feels like it needs to be a discussion with a mentor who can answer questions and highlight nuance. Unless a student is given good "whys" around this, it becomes an arbitrary and cargo-cult thing.

I'm not clear why or how adding in a class would or should impact the mutability aspect of the tests. The idea that functions should (generally) not mutate input data is distinct from classes.

That is true. But this exercise (right now) is fairly early in the progression and there is no intuitive reason for the student to expect that there be any sort of central "state" or "score" list (since the data is being passed in with each test).

Because there is no intuitive reason to look out for mutability issues (a scores constant, a global stash, an object attribute, etc), the immutability requirement is arbitrary. Why would a test validate object identity and look for mutated data? A general rule? But how does that help me with Python?

So it might be a better prompt to push the student into having a scores attribute in their constructed class that maintains scores (but could also be screwed up if sorted). Considering how Python treats attributes, that is certainly a Python-specific danger. Or a scores constant, if a class feels overloaded (because Python doesn't have constants, so there is danger there too). But I think there needs to be a clear thing that sets up and demonstrates the peril.

What are you hoping to see for the generated test code?

More or less what is there in the draft PR, with logic added to support some of the immutability test cases from canonical data. We may or may not also want to add tests either for a scores constant, or a scores attribute, depending on the implementation route. Once the template is done and tested, the test file needs to be regenerated.

I'm thinking we could either import and inherit the HighScore class and add <a>_after_<b> functions, which would call self.a() then return self.b(). Alternatively, the generated code could explicitly expand the <a>_after_<b> tests to call a() then test b().

Either works. I do think we need a few tests to make sure that the student is nudged toward having a scores attribute or constant or something similar. We want to set them up a bit so that they remember why mutating things might not work well.

[BethanyG]

Closing, as #3013 and #3016 are merged.