Grade-School: Align Instructions with Tests

Following up from this post.

I suggest adjusting the last sentence of grade school as follows in order to align with tests [1], [2]:

  • new: “In case someone attempts to add the same student more than once, this student’s grade should be updated.”
  • old: “In fact, when a test attempts to add the same student more than once, your implementation should indicate that this is incorrect.”

This originated from a mentoring session in TS where a student rightly (imho) noted that the tests did not match the description.

Additionally (but optional imo), I suggest also adjusting the second to last sentence since the language is a bit ambiguous at the moment:

  • new: “Note that no two students have the same name (It’s a small town, what do you want?) and each student can only be added to exactly one grade on the roster.”
  • old: “Note that all our students only have one name (It’s a small town, what do you want?) and each student cannot be added more than once to a grade or the roster.”

Happy to put this into a PR.

P.S. I’m wondering whether a change to the central description (which is propagated to tracks if I understand correctly) should lead to (automatically create?) a PR in all tracks, copying the previous central version into an override - this way track maintainers can decide to either update tests where necessary and drop the PR or merge the PR (or of course adapt an existing override in some way).

Tagging @erikschierboom here as suggested by @iHiD.

1 Like

This test indicates that grades are not updated when a student is added twice: problem-specifications/exercises/grade-school/canonical-data.json at b3832c8056803bbef304b357cdfa96757d7256e7 · exercism/problem-specifications · GitHub

The proposed update seems at odds with this test, if I’m reading it correctly.

PRs are not created automatically. Tracks maintainers need to do a sync.

Omg, you’re right, the TS tests don’t actually match the ones in the problem specification! So it’d have to be

  • new: “Any attempts to add the same student more than once should be ignored.”

That’s not really good “behavior”, though, so I wonder whether that’s really what we want to promote…

I like this suggestion.

1 Like

This is where it becomes tricky, as the same instructions are shared across tracks. I agree that “should be ignored” might not be best way to phrase this, as some tracks might want to error or do something else. Could we come up with a track-agnostic description?

Just to be sure: how much should the tests in individual teacks correspond to the test specifications? These currently don’t error, but ignore the subsequent calls to add.

I guess in any case we’ll have to change the tests in TS since these seem to just be “wrong”…

Well, we don’t enforce anything. Tracks are free to deviate form instructions if that makes sense for their tracks. Most of the time, tracks stick quite closely to the instructions though.

You could look through the tracks and see how many implement the “ignore new adds” behavior. If a large majority do this, we may want to simply update the text to follow what is implemented already vs introducing “breaking” changes.

Ok, so I guess what it boils down to is:

  • new "Any attempts to add the same student more than once should either be ignored or there should be an indication that this is incorrect - in a true TDD manner, check the tests to see what’s expected! .”
  • We also change the last sentence as suggested above
  • We change the TS tests to match the description

Agreed? If so, what do I do to make sure my PR s are not auto-closed?

  1. Wait for consensus here.
  2. After there is a consensus about the best approach, open a PR.
  3. PR will be auto-closed. Link to the PR here and someone will reopen it.
1 Like

Of the 68 tracks I have open, 40 implement grade-school. 24 of those implement 66c8e141-68ab-4a04-a15a-c28bc07fe6b9.

  • awk expects the second add to be a no-op (ignored)
  • c common-lisp crystal csharp elixir elm expects the second add to return a value indicating the student could not be added and have no side effect (no exceptions, no change to the roster)
  • I stopped there and did not check erlang fsharp gleam java nim perl5 powershell prolog python racket raku ruby swift tcl vbnet vlang wren

Based on the above, I think tests should probably ask that solutions not change anything on a second “add” and instead (where supported) return a value indicating the student is a duplicate and cannot be added.

1 Like

I just encountered this issue on the JS track, so I just wanted to mention it here (as I imagine JS is one of the more popular tracks and will get a lot of students coming across this issue). It’s exactly the same as the issue described with TS:

  • The instructions specify that when a test attempts to add the same student more than once, your implementation should indicate that this is incorrect.
  • The tests actually require that when a student with the same name is added a second time, their grade is updated to the new grade argument (and removed from the previous grade). The tests do not expect any errors or special return values.
  • The actual test differs from the “global” exercise spec, although the global spec isn’t visible anywhere in the exercise, so I don’t think this is the source of any confusion, but rather the discrepancy between the instructions and the tests.
1 Like