problem-specifications icon indicating copy to clipboard operation
problem-specifications copied to clipboard

Should the value of `error` be mutable?

Open ee7 opened this issue 5 years ago • 4 comments

Example

In perfect-numbers we have this test case: https://github.com/exercism/problem-specifications/blob/8df6cb2ed8e77aa3ac0c02744cd9ddb83920f5f2/exercises/perfect-numbers/canonical-data.json#L121-L130

And we notice (see https://github.com/exercism/problem-specifications/pull/1691) that we would like to make this change:

 { 
     "uuid": "72445cee-660c-4d75-8506-6c40089dc302", 
-     "description": "Zero is rejected (not a natural number)", 
+     "description": "Zero is rejected (not a positive integer)", 
     "property": "classify", 
     "input": { 
         "number": 0 
     }, 
     "expected": { 
-         "error": "Classification is only possible for natural numbers." 
+         "error": "Classification is only possible for positive integers." 
     } 

We seem to have decided that description is allowed to mutate when the "meaning is unchanged" (that is, we do not add a reimplements test case for e.g. a simple description-only typo fix).

The question is: should the contents of an error also be considered mutable when the "meaning is unchanged", or should that require a new test case?

Discussion

My initial opinion is something like:

  1. We said that description should be mutable if we don't change the "meaning" of the test.
  2. There are some useful error-message changes that don't change the "meaning" of the test either, and the value of error is similar to the "description" of the error. In the limiting case, it seems messy to add a new test case just to fix a typo in an error value.
  3. Therefore, directly mutating the error value should similarly be allowed when the "meaning is unchanged".

Drawbacks:

  • Is there a track that asserts something about the contents of the error message for this kind of test? Such a track would have to update that assertion, even though their tests.toml didn't change.
  • The CI check for immutability would need to check that expected doesn't change, but make a special case for an error-only change.

ee7 avatar Oct 15 '20 10:10 ee7

I'd vote: it's mutable, except if the mutation would change the total set of errors in an exercise. Using your example, I'd expect a Rust test generator to come up with

enum Error {
  ZeroIsRejectedNotANaturalNumber,
  ClassificationIsOnlyPossibleForNaturalNumbers,
}

If a PR updates the text on all such errors, we now have

enum Error {
  ZeroIsRejectedNotAPositiveInteger,
  ClassificationIsOnlyPossibleForPositiveIntegers,
}

The set of errors, and the semantics, hasn't changed; only the enum's label. I think this is fine.

If, on the other hand, we rewrite things to enum Error had some number of variants other than 2, then that's a breaking change; that should be added as a new test, because there are now new semantics in the exercise.

Make sense?

coriolinus avatar Oct 15 '20 11:10 coriolinus

It may help to compare the implementations of one exercise involving errors across tracks to find out if they'd be breaking or not. I don't know if any tracks check for an exact error message for example.

SaschaMann avatar Oct 15 '20 12:10 SaschaMann

Okay, I think we have an agreement that the error property in the expected property is allowed to be updated without introducing new test cases, but on certain conditions. @coriolinus phrased those conditions well. Would someone be interested in updating the documentation?

ErikSchierboom avatar Oct 15 '20 13:10 ErikSchierboom

JavaScript often checks for an exact message; especially in older exercises. In newer exercises we usually check for class name, or better, provide it via the stub.

SleeplessByte avatar Dec 16 '20 22:12 SleeplessByte