high-scores: add tests to prevent solutions that mutate 'scores'

[mfeif]

On the exercise 'high-scores', I'd say 80-90% of the people I've mentored on that one mutate 'scores' in one or more of the functions. It's a subtle but easy thing to miss, and it's compounded by two gotchas that can happen at once: the pass-by-reference thing, and the fact that some methods on lists DON'T return a copy, but operate in place.

As is, the tests can pass if someone damages scores by sorting it in place, or even pop()-ing stuff from the list (both stuff I've seen). But I'd say that would be a bad habit to allow.

Here is a patch that adds tests to make sure that after running all three functions, scores is still the same. This would save lots of mentor time, and might encourage students to find the facts about list methods on their own. I added three tests rather than just one that tested the same 'bug' on all three methods, because it seems that the small units are the convention.

[mfeif]

Oops. I have just seen that someone else in that slack thread also went and solved this problem with #1873. I guess we got our wires crossed.

I would suggest though, that we don't need deepcopy() and I'd hate for beginners to get distracted with that.

[mfeif]

Hmm. Looks like CI is demanding that I generate these tests from metadata. I can't find that metadata, though I can find the jinja template. (Now I see that's in a different repo)

I understand the notion of trying to systematize this stuff, but it means that we now have to general-case into the macro all the special cases that might come up. That doesn't seem to necessarily be a step forward. I've found and read #1857 but I don't understand what to do in order to contribute.

That template seems to allow only a simple sort of test; anything that might require even one more line beyond the assertEquals() would be out of bounds. Ergh.

[yawpitch]

Hi @mfeif, I appreciate the PR, but we decided some time back that mutation was acceptable in high-scores, given its position in the core of that track. See discussion in #1868, #1745, #1735, and earlier. There is an open PR in the website-copy repo that is meant to make that explicit in the mentor notes as well.

The test generator is an early work in progress, meant to translate updates in the canonical data into test suites in order to make it easier to keep the Python track up to date with changes upstream. It's not yet fully fleshed out. Generally though if there are special cases yes, for the moment, they'd need to be added to the template, not the canonical repo.

[mfeif]

Argh. I should have searched more fully in the closed issues.

I've wasted hours and hours talking to students about immutability, mostly also based on the suggestions. I see that everyone understands this and a clear solution hasn't come up yet.

Thanks. I'll withdraw this PR.

So if the tests have to be generated, they why is there a test file in each exercise in the repo? And what if a hint or something needs to be added with a docstring or a comment?

[yawpitch]

The test file must exist because that's what's ultimately distributed to students. The generator builds a new test file from an updated canonical-data.json, then we manually verify that everything worked and commit the new file.

As for hints and docstrings; odds are good that anything that requires such a hint probably needs it in the canonical data, rather than it being directly added to the track's test artefacts. But as of right now you'd end up having to add the comment to the template.

And don't get us wrong on this one, students definitely need to learn about immutability, it's just that high-scores is too early in the core for that, and honestly the core hasn't gone through enough iterations to get us to the point where it's teaching what needs to be taught in the correct order. This is meant to be addressed via the Track Anatomy epic (#1867) but it will take some time.

[mfeif]

Cool. Thanks for your thoughtful responses.

[mfeif]

I would still say that this hugely increases the friction for contribution; now any ideas that I have that would improve the learning experience need to map across -- what? 40 languages? I'm not familiar with the details of 40 languages; how many are pass by reference? I dunno.

Also, I would guess that I need a build system to make tests now.

I see that this is a done deal, I suppose I'm just expressing sadness that now I know that I can't contribute, or that the cost of doing so is beyond my skill, though I have 20+ years experience programming in python.

When do the new guidelines get posted to our control panels as mentors? I see that there is work on that, which is great... do the changes get pushed to the live website on a schedule, or as changes are approved or...?

[yawpitch]

There’s plenty of room for contribution; the main additional barrier is Jinja2, which exists to reduce what is the currently quite high maintainer overhead of keeping the exercise test suites up to date. There are very few PRs opened for Python-specific improvements to tests, mostly the Python track benefits from changes to the canonical data raised by other tracks. Essentially there’s 40 language tracks finding edge cases in the tests the canonical data describes, and when an upstream change is made we usually want to roll it out to the track ASAP. Right now that’s an entirely manual task and one that distracts us from things like Track Anatomy improvements and developing auto-mentoring.

So the amount of friction depends on what change you’re hoping to make. If it’s a direct change to a test file for a Python-specific test then yeah, you’ll need to make that in the Jinja2 template, if one exists for the exercise, but that’s really not very hard, as you’ll probably need to use only the most minimal Jinja2 templating, if any. Updates to other files will not have that overhead. But the templates will save a lot of work on the majority of test changes, which will come from upstream.

There a https://github.com/exercism/python/blob/master/requirements-generator.txt file to help you build a virtualenv and get up and running. You’ll just need a relatively recent Python 3 interpreter installed to use that.

Of course you can also just make changes to the test file directly and submit a PR... if it’s in an exercise with a template we can help get the changes into the right place. The CI will fail till that’s done, but that’s fine, it’s just an indicator that those changes need to be loved into the template.

As for guidelines I’m not sure of exactly how long it takes to go live after merge, but it’s relatively quick; IIRC it’s a CI job that’s triggered by merge to master. @cmccandless might have a more specific idea.

[cmccandless]

Changes to test suites are live within a few hours.

[mfeif]

@yawpitch thanks again for your patient explanation. I can grok Jinja; I thought that one had to modify the upstream as well. I'm glad that you're working on a solution to make maintainers' lives easier.

@cmccandless thanks also; I see that the new text is now live on the site; I guess all of this is happening just as I happen to wade in.

Thanks folks.

[cmccandless]

The Jinja stuff is brand new, and will get some contributor documentation very soon.

[yawpitch]

@yawpitch thanks again for your patient explanation. I can grok Jinja; I thought that one had to modify the upstream as well. I'm glad that you're working on a solution to make maintainers' lives easier.

Sorry, should have been more clear there. There's a reasonable chance that an edge case discovered in any given test file is a general issue that applies to the problem as a whole and not to a specific language; these should be pushed to the canonical data, so all the language tracks can benefit from it. If however the issue being addressed by a proposed test is language-specific then it gets applied at the language level.

Which is which will sometimes be a judgement call, so if in doubt start a new Issue before starting work on a Pull Request.