Freecodecamp: Formatting issue within Basic Javascript, Record Collection

[skaparate]

Describe your problem and how to reproduce it:
For the challenge Javascript Algorithms and Data Structures -> Basic Javascript -> Record Collection, the collection variable is not supposed to change, but whenever you format the code (Control + Shift + i) the collection changes, which will fail the challenge and since it only removes single space character it's hard to notice.

To reproduce

1. Go to the challenge.
2. Press Control + Shift + i to format the code.
3. Run the tests. The first test fails with You should not change the collection object's initialization, even though the user didn't really modify it.

Add a Link to the page with the problem:
https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures/basic-javascript/record-collection

Updating the challenge to remove the space and fixing the RegEx should solve the issue:

var collection = {
  2548: {
    album: "Slippery When Wet",
    artist: "Bon Jovi",
    tracks: [
      "Let It Rock",
      "You Give Love a Bad Name"
    ]
  },
  2468: {
    album: "1999",
    artist: "Prince",
    tracks: [
      "1999",
      "Little Red Corvette"
    ]
  },
  1245: {
    artist: "Robert Palmer",
    tracks: [ ] // <-- Remove this space
  },
  5439: {
    album: "ABBA Gold"
  }
};

And update the test/regex accordingly.

[rajatgupta24]

well, if you do

 `console.log(collection[1245]["tracks"]);`

you'll get (in the freecodecamp console)

 `undefined `

i.e. when you enter the value in it, it will be "collection[1245]["tracks"][0]", there is no space.
there must be some other error, if i get it correctly.

thank you

[skaparate]

well, if you do

 `console.log(collection[1245]["tracks"]);` 

you'll get (in the freecodecamp console)

 `undefined `

i.e. when you enter the value in it, it will be "collection[1245]["tracks"][0]", there is no space.
there must be some other error, if i get it correctly.

thank you

Hello :smile:!

I tested what you mention on (Ubuntu, Linux) Firefox, Opera and Chrome, and none of them printed undefined, neither on the FCC console nor the browser's console:

Chrome

Firefox

Opera

Anyway, that wasn't the problem, sorry if I explained it wrong.

Read this thread on the forum. The user (venkatpantham), without modifying the initial collection variable, wasn't able to pass the test because he may have formatted the code.

See the following image:

The highlighted text shows that the first test is passing. Now, after formatting the code (Control + Shift + i on the FCC code editor):

The only portion of code that was modified was the tracks property of the 1245 object, and highlighted is the code message from the test.

I hope this is clearer now :sweat_smile:

[moT01]

I could not reproduce this. But a fix would be to change the test I believe. Here are the tests for this challenge.

The first test could be changed to allow a space or not...

- testString: 'assert(code.match(/var collection = {\s*2548: {\s*album: "Slippery When Wet",\s*artist: "Bon Jovi",\s*tracks: \[\s*"Let It Rock",\s*"You Give Love a Bad Name"\s*\]\s*},\s*2468: {\s*album: "1999",\s*artist: "Prince",\s*tracks: \[\s*"1999",\s*"Little Red Corvette"\s*\]\s*},\s*1245: {\s*artist: "Robert Palmer",\s*tracks: \[ \]\s*},\s*5439: {\s*album: "ABBA Gold"\s*}\s*};/g));'
+ testString: 'assert(code.match(/var collection = {\s*2548: {\s*album: "Slippery When Wet",\s*artist: "Bon Jovi",\s*tracks: \[\s*"Let It Rock",\s*"You Give Love a Bad Name"\s*\]\s*},\s*2468: {\s*album: "1999",\s*artist: "Prince",\s*tracks: \[\s*"1999",\s*"Little Red Corvette"\s*\]\s*},\s*1245: {\s*artist: "Robert Palmer",\s*tracks: \[\s*\]\s*},\s*5439: {\s*album: "ABBA Gold"\s*}\s*};/g));'

The change is towards the end there. What do you guys think @skaparate @rajatgupta24?

[Sky020]

@moT01 , Surely it would be more robust to create a collection object, and compare them:

testString: 'assert(JSON.stringify(a) === JSON.stringify(b));'

[skaparate]

@moT01, yes, the test should be changed but I would change it using @Sky020's proposal :grin:.

[moT01]

hmm, wouldn't the collection object have changed?

[ojeytonwilliams]

Comparing objects via stringify can fail, since there's no guarantee of property order. assert.deepEqual should do the trick.

[skaparate]

hmm, wouldn't the collection object have changed?

Yes, you're right, it will change after the student defines and executes the function, but I suppose we should be parsing only the initial collection var at that point, no?

[Sky020]

Might as well make use of deepEqual, seeing as it is more robust. I do have a qualm with the challenge asking the user to mutate the object in the way it does, but that is for another day.

[skaparate]

Hello!

I tested the challenge using assert.deepEqual, but it seems to be unreliable too. I suppose because we cannot guarantee the order in which the tests are run and after the solution is written, sometimes all tests pass and sometimes the first test (the one in question) fails.

The solutions I can think of are:

• Fix the RegEx,
• Impose/teach good practices and ask the students to clone the collection object, instead of modifying it (or pass the collection as argument). I think this can confuse students that are just starting, unless we taught them about mutability (which, if I recall correctly, is taught somewhere later).

What do you think @moT01 @ojeytonwilliams @Sky020?

[Sky020]

@skaparate , Is there a simple way to take a snapshot (clone) the users collection object, before the tests are run? Then, we could use that for comparison with a testCollection object.

I do not understand how the current test is more reliable than assert.deepEqual, if the tests are run at different times?

Whilst I am for teaching _good practices_, I do not see that being necessary to get around fixing this one test.

[skaparate]

@skaparate , Is there a simple way to take a snapshot (clone) the users collection object, before the tests are run? Then, we could use that for comparison with a testCollection object.

I do not understand how the current test is more reliable than assert.deepEqual, if the tests are run at different times?

Whilst I am for teaching _good practices_, I do not see that being necessary to get around fixing this one test.

Yes, we can, just like the Spanish solution, which basically copies the object after it is seeded... I was just overthinking it.

[ieahleen]

the JSON.stringify way of doing was removed from the challenge in #36002 and it was discussed in #35990, because doing that way would let users just change the original object declaration.

Yes, we can, just like the Spanish solution, which basically copies the object after it is seeded... I was just overthinking it.

the spanish solution has simply not yet been updated

[Sky020]

@Ieahleen , My idea for implementation is:
1) Create object testCollection equal to the original seed of collection
2) Compare testCollection against collection at test run
3) If user has changed collection, testCollection !== collection and first test fails.

[skaparate]

@Ieahleen I see. Though, what I understand from the discussion is that after removing the line var collectionCopy the problem arose, but I understand the point @RandellDawson proposed there (the student hasn't been taught about JSON.stringify|parse, which may confuse them).

So, taking into account Randell's point I should check using a regex instead, do we agree?

[lasjorg]

Shouldn't the regex just be updated to have space in the array be optional? What is the issue with the current test other than that?

[skaparate]

Yes, it should be optional.

What is the issue with the current test other than that?

None other AFAIK.

[lasjorg]

@skaparate Do you want to make the PR?

I don't see why we shouldn't just fix this. If anyone comes up with something better we can always implement that.

[skaparate]

Sure :smile:, I'll create a PR.

[RandellDawson]

It seems like we could get rid of the regex all together by writing the testString as:

  - text: You should not change the <code>collection</code> object's initialization.
    testString: assert.deepEqual(collection, _recordObject);

and then referencing the _recordObject in an"After Test" section:
````

After Test

const _recordObject = {
  2548: {
    album: "Slippery When Wet",
    artist: "Bon Jovi",
    tracks: [
      "Let It Rock",
      "You Give Love a Bad Name"
    ]
  },
  2468: {
    album: "1999",
    artist: "Prince",
    tracks: [
      "1999",
      "Little Red Corvette"
    ]
  },
  1245: {
    artist: "Robert Palmer",
    tracks: []
  },
  5439: {
    album: "ABBA Gold"
  }
};

````

I tested this locally and it seems to allow the user to format the object anyway they want as long as they do not change the properties and values. Since I wrote this rather quickly, I may be overlooking an edge case.

[skaparate]

@RandellDawson this is weird... your solution doesn't work when validating the challenge with a solution (it works if no solution is provided), but it passes the tests when running npm run test:curriculum --block='Basic JavaScript: Record Collection'.

I tried to implement this test:

- text: You should not change the <code>collection</code> object's initialization.
testString: assert.deepEqual(_parsedCollection, _recordObject);

With this teardown:

````

After Test

const _parsedCollection = JSON.parse(
  code.replace(/.+var\s+collection\s*=\s*(\{.+\});.+/gs, '$1')
  .replace(/([a-z0-9]+):/gi, '"$1":'));
const _recordObject = Object.freeze({
  2548: {
    album: "Slippery When Wet",
    artist: "Bon Jovi",
    tracks: [
      "Let It Rock",
      "You Give Love a Bad Name"
    ]
  },
  2468: {
    album: "1999",
    artist: "Prince",
    tracks: [
      "1999",
      "Little Red Corvette"
    ]
  },
  1245: {
    artist: "Robert Palmer",
    tracks: []
  },
  5439: {
    album: "ABBA Gold"
  }
});

````

Which works when a solution is provided (through the actual challenge page) but fails the tests with _parsedCollection is not defined (when running the tests through docker) but no other error displayed. I tested the regex on the browser console and it fails too, but it doesn't fail on node nor inside a script tag inside an HTML file... weird, I think. This is not better than the original solution and I even think that modifying the original regex is better than this (as the proposed PR does).

[RandellDawson]

@skaparate I will investigate further and see if I can reproduce what you are seeing.

[RandellDawson]

I just tested my proposal again and ran npm run test -- -g 'Record Collection' after first changing to the curriculum directory and it works as it should.

I tested the regex on the browser console and it fails too, but it doesn't fail on node nor inside a script tag inside an HTML file... weird, I think.

@skaparate I am not understanding what you mean by "tested the regex on the browser console". Can you elaborate on what you mean here? What exactly did you enter into the browser console?

As far as the docker error, I do not use docker, so I am unable to help there. @ojeytonwilliams Can you shed some light on what could be causing the docker error?

Also, before running any of the tests, that everything is up to date when using docker

git pull --rebase upstream master
cp sample.env .env
npm run docker:clean
npm run docker:install
npm run docker:seed
cd curriculum
npm run test -- -g 'Record Collection'

[Sky020]

@RandellDawson Your solution works well to resolve this issue.

[skaparate]

I just tested my proposal again and ran npm run test -- -g 'Record Collection' after first changing to the curriculum directory and it works as it should.

Sorry! I didn't explain it correctly.

Using your solution, the tests pass when run through npm or docker, but do not work if you try to submit the solution through the challenge page on the browser (localhost:3000/learn/javascript-algorithms-and-data-structures/basic-javascript/record-collection). Maybe it was just me, but I started from scratch (pulled all from upstream and even removed all the files) and tested on Firefox (where I do have plugins/addons), Opera and Chrome (which don't have any plugins).

I am not understanding what you mean by "tested the regex on the browser console"

Don't worry, it's not an issue with your solution, it was more like a has it happened to you? (I now know it's related to the s flag of the regex, so I'll test later today). The main issue is with my solution; it simply doesn't pass the tests (I know it's the regex, but it works fine on node, so I don't know what's happening).

[skaparate]

I tested again, but this time using gitpod with the same result. Here's a GIF of what I did (it's a little big :sweat_smile:): https://drive.google.com/file/d/1XWv3-uu1ZhtLP99w3z9jlPIafPrPBK8Q/view?usp=sharing

[RandellDawson]

@skaparate After watching your video, I now know what the issue is. When I was testing it, I only used the function and not the function and the call to the function (which is part of the seed code). If you comment out the call to the function, it works as expected. However, we do not want a test to fail just because of a user making a call to the function while they are testing things out.

The reason calling the function with updateRecords(5439, "artist", "ABBA") causes it to fail, is based on how our test runner works. Each test is completely isolated from each other. However, for each test all the code in the editor (the function plus the call to the function) is executed before each test runs. That means, when the first test runs, the call to the function updates collection before the testString is evaluated. So, it will never be "equal" to __recordObject, because "artist": ABBA" gets added to the property 5439 before the assert.

I will need to ponder this overnight. If I can not figure out a way to resolve, then we will have to move forward with a regex approach.

[Sky020]

@RandellDawson To close both this and the related #38962 , why not try something similar to: https://github.com/freeCodeCamp/freeCodeCamp/pull/39385

We could pass _recordObject to the function for each test. This would probably require the lesson and seed to be changed to have the collection passed as an argument.

Proposed seed:

// Same old object for reference...

// Only change code below this line
function updateRecords(object, id, prop, value) {


  return object;
}

updateRecords(5439, "artist", "ABBA");

That, or similar. I realise this is a lot more work...

[Sky020]

@skaparate What are your thoughts about:
1) Change the seed so that the function accepts an object, and returns the object changed.
2) Remove the first test (no need for it)
3) Run all tests, by passing _recordObject to the function.
4) Leave the collection variable as is, so campers can use it to debug.
5) Change lesson text to clarify: _"The original collection object will be used for the tests"_

[skaparate]

@Sky020 Nice! I'll fix it and create the PR. Thanks :smile: