Skip to content

Conversation

@paparomeo
Copy link
Contributor

@paparomeo paparomeo commented Jan 4, 2020

Adds a test to the gigasecond exercise make sure the input is not mutated, since there are some solutions which will pass the current test suite that will mutate the input, which is undesirable. I've now encountered at least twice while mentoring solutions that appear to be correct, but mutate the input, so it makes sense to include this case in the test suite.

Added directly here instead of the problem specifications repository, because unfortunately it's a test case that cannot be defined generically.

@paparomeo paparomeo changed the title Add a test to check that input is not mutated [gigasecond] Add a test to check that input is not mutated Jan 4, 2020
@ovidiu141
Copy link
Contributor

ovidiu141 commented Jan 4, 2020

Mutating the input and then going back to the initial value will make the test to pass (but I've still made a mutation):

  inputDate.setDate(15); // random mutation
  inputDate.setDate(4); // going back to the original value

So using toBe() instead of toEqual() might be a better idea since it tests if it's still the same object, not just the same value.

@paparomeo
Copy link
Contributor Author

Interesting point @ovidiu141! However, I'm not aware of any way to actually change the input argument to be a different object (if you assign to an identifier with the same name you'll just be creating a shadow local variable with the same name). So the most that can be done is internal mutation of the argument object and therefore I think we can only check if the object has the same value coming out as it did going in. Please correct me if I'm wrong.

@ovidiu141
Copy link
Contributor

ovidiu141 commented Jan 5, 2020

@paparomeo I agree. I am also not aware of any way to change the reference of an input argument, so toBe() seems to not be the answer here. I was wrong.

In this case I think it's impossible to have a test which really tests if the input object is not mutated at all. Your test is good enough, but does not cover the case I mentioned above.

Example showing how your test passes, even if mutation is made:

const gigasecond = date => {
    date.setDate(15); // Actual mutation
    date.setDate(4); // Another mutation, but going back to the original value
};

test('does not mutate the input', () => {
    let input = new Date(Date.UTC(2020, 0, 4, 20, 28, 30));
    gigasecond(input);
    expect(input).toEqual(new Date(Date.UTC(2020, 0, 4, 20, 28, 30)));
});

@paparomeo
Copy link
Contributor Author

I dug a little deeper into this one, thinking that if Date instance could be frozen we could approach the test from that angle, but unfortunately it seems they aren't, so I think this is the best we'll achieve. I think it's good enough, since the main goal is to prevent students from incorrect using date.setUTCSeconds to solve the exercise.

@SleeplessByte
Copy link
Member

SleeplessByte commented Jan 7, 2020

Hi there!

I've thought about this for a bit. In order to accept this change, I want to add a few lines to the README.md, via a HINTS file. It has to state that the input must stay unaltered. Otherwise we are testing for implementation without guiding the student in the original description that it shouldn't change.

Follow these instructions to add a hint file and regenerate the README. Once that is done, please also create an issue in the javascript-analyzer repository so we can automatically test for this (we already do !!, but then it can be ensured as opposed to suggested).


As for the solution, mutation is perfectly fine, since JavaScript is single-threaded, as long as the input is the same "after" the function has ran as "before" it has ran. So a simple toEqual is fine.

@SleeplessByte SleeplessByte added the enhancement 🦄 Changing current behaviour, enhancing what's already there label Jan 7, 2020
@paparomeo paparomeo force-pushed the test-input-mutation branch from f68e0be to a577f24 Compare January 8, 2020 22:02
@paparomeo
Copy link
Contributor Author

paparomeo commented Jan 8, 2020

@SleeplessByte: .meta/hints.md file added with hint about argument mutation and README.md regenerated. exercism/javascript-analyzer#62 created.

@SleeplessByte SleeplessByte merged commit 00ad491 into exercism:master Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement 🦄 Changing current behaviour, enhancing what's already there

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants