isolate tests in exercise

[shieldo]

The tests here are not isolated in a way that I would expect, as the list of article titles was passed by reference to the functions, which means that any mutations that functions implemented for the exercise make on the parameter mutates the fiture for the other tests. This can mean that trainees get other tests failing in ways they did not expect.

It could be argued that there is a teachable moment here potentially about mutating parameters, but I'd make the suggestion with this PR that the syllabus hasn't yet taught about the difference between scalars and objects in terms of passing by reference or value, and trainees may well have come across occasions where parameters were mutated but this was not expected to matter because the expected types passed were numbers or strings. For the most part, trainees experiencing an issue here will be presented with tests failing with no clear idea why. I think it's better to ensure that tests are isolated and not to care too much about mutation at this stage.

[sztupy]

What about doing instead

Object.freeze(ARTICLE_TITLES);

This would cause the trainee receiving a object is not extensible exception in case they accidentally try to push values into this constant.

(note I come from ruby where it is usually expected to explicitly freeze constants so they become inmutable, not sure if this is as well known in the JS world or not)

[shieldo]

Thanks for this feedback!

I think I'm against this for two reasons:

1. I'm making the argument here to make mutability not a problem at this stage (so as to not overload the exercise) - tests like this should be isolated because the order in which they execute is non-deterministic, so this situation has arisen out of bad practice. It's reasonable that mutations are commented on I think in code review, but not reasonable that they make code fail with an error that assumes a lot of knowledge for this stage

2. I'd argue that tests should always be reasonably legible to trainees at whatever level of the course they are at, and referencing the Object class I'd argue is premature. If trainees were to look up Object.freeze and understand what it did, I'm not sure that the reason for using it here would be clear.

I wasn't aware of Object.freeze in JavaScript, actually, and am used to calling .freeze on objects in Ruby that are constants! The equivalent thing to do here I think would be to freeze the array on assigning it to ARTICLE_TITLES, but I'm not convinced it's the right approach.

[sztupy]

I believe we should teach the trainees that mutability is usually not accepted, even if not explicitly stated on the exercise level at least in the tests - the tests should explicitly check that the array that was sent in remained unchanged. If any trainees gets blocked by this they could always ask. On the other hand if they send in a solution which does change the input and no one tells them that's usually not acepatable they could get lost later. I actually did receive a couple questions related on this on why they needed to create a new array in these solutions, although they could just change the parameter in one of the week2 solutions (https://github.com/CodeYourFuture/JavaScript-Core-1-Coursework-Week2-Solution/blob/main/mandatory/2-function-creation.js#L81 ) which was a good question on multiple fronts (immutability, variable assignments, difference between changing an object and assigning a new value to a variable, etc)

On point 2, that could always be solved with a simple comment, like // make sure that the list of article can not be accidentally changed. But as you mentioned freeze is not as known in JS land, so I'm happy to swap it's usage to explicit checks that the functions created are pure and don't have side effects

[shieldo]

@sztupy I've added some extra checks in the tests that arguments have not been modified, while keeping the isolation - this would have the advantage of giving useful error messages when a modification does happen within the same test.

I'm not sure whether this introduces too much boilerplate, but it seems to check the box.