Grade-School: Align Instructions with Tests

Exercism Building Exercism
1

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).

2

Tagging @erikschierboom here as suggested by @iHiD.

1 Like
3

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.

4

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…

5

I like this suggestion.

1 Like
6

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?

7

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”…

8

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.

9

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.

10

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?

11
  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
12

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
13

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