Pull Requrest on exercism/javascript

Hello,

I know that community contributions are currently in pause. But I do have a small merge request that I think is interesting to merge (I just saw a solution that was wrong but where the test suite did not fail).

Here is my merge request: Matrix: Ensure whole array is correct by homersimpsons · Pull Request #2065 · exercism/javascript · GitHub

1 Like

It generally helps a lot in threads like these to be very explicit, so that it is very easy for someone passing by to understand what your MR is about. Currently they have to figure it out themselves.

Can you provide specific examples of plausible solutions that pass but should fail?

In general, I expect added tests to be way more likely to be adopted than changes to tests.

Hello @MatthijsBlom ,

Thank you for your answer.

Can you provide specific examples of plausible solutions that pass but should fail?

For instance the following solution will fail nearly all newer columns tests while it currently pass those tests:

export class Matrix {
  constructor(matrix) {
    this.matrix = matrix
  }

  get rows() {
    return this.matrix
      .split('\n')
      .map(row => row.split(' ').map(entry => +entry));
  }

  get columns() {
    const columns = [];
    for (let i = 0; i <= this.rows.length; i++) {
      columns[i] = this.rows.map((row) => {
        return row[i];
      });
     }
     return columns;
  }
}

The issue is that the user is iterating on this.rows.length instead of this.rows[0].length.

Those are the failing tests:

// extract column from one number matrix
expected: [[1]]
received: [[1], [undefined]]
// can extract column
expected: [[1, 4, 7], [2, 5, 8], [3, 6, 9]]
received: [[1, 4, 7], [2, 5, 8], [3, 6, 9], [undefined, undefined, undefined]]
// extract column where numbers have different widths
expected: [[89, 18, 9], [1903, 3, 4], [3, 1, 800]]
received: [[89, 18, 9], [1903, 3, 4], [3, 1, 800], [undefined, undefined, undefined]]

In general, I expect added tests to be way more likely to be adopted than changes to tests.

I can update the Merge Requests to add those tests instead of updating the current ones. I was just thinkig that as those are precisions to the tests and alligned with the instructions it would make more sense to just update the current ones.

Do you want me to add those as separate tests ?

I am not a maintainer of the JS track.

The reason I expect additions to be more welcome than changes is because problem specifications (including tests) are often shared between many tracks. Therefore, ‘fixes’ of the data should generally be submitted to exercism/problem-specifications. From there changes will flow down to individual tracks. This process is much easier when changes are small, which additions tend to be. Additions also preserve consistency.

I am not a maintainer of the JS track.

Do you know anyone I can mention here ?

problem specifications (including tests) are often shared between many tracks

In fact, it looks like it is the case for these cases: exercism/problem-specifications/blob/fa6cd72fa4e037cb97119fa003adac382906c3cf/exercises/matrix/canonical-data.json (I cannot post the github link)

So it could make more sense to add new tests. But those may be duplicates of the previous one and I can easily expect this mistake to happen in other languages.

Hello there!

Okay, you did a few things right:

When tests are changed or added we always explore:
- does it fix a bug, as in, does it capture an incorrect implementation, and if so, which
- does it unnecessarily make the exercise harder
- does the change hold enough value to generate churn

You have pointed out which solution should fail the test and currently doesn’t, without the added tests making the exercise harder.

In that case, it’s a good idea to open a PR on problem-specifications. In general, we will accept tests that abide by the above rules. Include the JS solution that fails the new tests in that PR. Once that is merged, JavaScript (aka me, june, tejasubane) will be happy to merge a follow-up PR.

The reason we do this for practice exercises is that a lot of tracks have implemented these and they may see issues or solutions that we don’t. For example, maybe the problem persists in some other language unless we add yet another argument.

I hope this helps!

Hello @SleeplessByte,

In that case, it’s a good idea to open a PR on problem-specifications. In general, we will accept tests that abide by the above rules. Include the JS solution that fails the new tests in that PR.

I just opened the Pull Resquest: exercism/problem-specifications/pull/2195 (still cannot post GitHub links …)

Once that is merged, JavaScript (aka me, june, tejasubane) will be happy to merge a follow-up PR.

Perfect, hopefully this would get merge soon. I guess it is hard for mentee to see that they made a mistake while all the tests pass.

I hope this helps!

Yes, thank you for the additional context.

1 Like

It’s off-topic, so someone like Jonathon can delete this when it’s resolved, but @homersimpsons what is preventing you from posting a GitHub link? Do you get an error message? If so, what’s the error message?

Yes I have an error message that says that I am self-promoting, then I receive a message in my inbox to tell me that a moderator should approve it.

(Quick test to trigger the warning again: Matrix: Ensure whole matrix is correct by homersimpsons · Pull Request #2195 · exercism/problem-specifications · GitHub)

Looks like I did not have the message there. If I have it in the future I will post about it.

Hello @SleeplessByte

Any hope my Pull Request get re-opened ? Should I ping someone specifically ?

I had a very similar experience and opened Pull Request 2049 against the JavaScript repository recently.

The context was that coming from other programming languages, the scope of the cached values may be surprising in JavaScript. When looking at community solutions, I was certainly surprised that other folks were not testing whether f had changed between invocations of the closure returned from memoizeTransform().

My change was to add a new test case asserting that different values of f have different caches.

I hope that it can be merged.

Hello @johnsyweb

You certainly want to link you Pull Request here so it get attention: Ensure recalculation when a new function is passed in by johnsyweb · Pull Request #2049 · exercism/javascript · GitHub

Thank you.

I’ve re-opened it. It’s a good addition to the exercise.

@johnsyweb for a future PR, it would be good to include the faulty solution that passes the old tests and doesn’t pass the new tests. Further we always check two things:

  • Does this unnecessarily complicate the exercise (no); if yes we will likely not approve
  • Does this require a change to the proof and canonical solution (no); if yes, it will need to be changed too