Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the value of error be mutable? #1709

Open
ee7 opened this issue Oct 15, 2020 · 4 comments
Open

Should the value of error be mutable? #1709

ee7 opened this issue Oct 15, 2020 · 4 comments

Comments

@ee7
Copy link
Member

ee7 commented Oct 15, 2020

Example

In perfect-numbers we have this test case:

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

And we notice (see #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.
@coriolinus
Copy link
Member

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?

@SaschaMann
Copy link
Contributor

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.

@ErikSchierboom
Copy link
Member

ErikSchierboom commented Oct 15, 2020

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?

@SleeplessByte
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants