Syncing test.toml and updating the test code for reverse-string

[jagdish-15]

Pull Request

This PR syncs tests.toml with the problem-specifications repository and verifies the presence of new tests in the test file.

[github-actions]

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[jagdish-15]

This change introduces grapheme cluster & wide character handling to the exercise, which would require updating the proof solution accordingly. We had previously decided to skip such changes in the C track, see this PR for context. Could you please confirm if we’re moving forward with this change here?

[Cool-Katt]

Well ,there is a way to work with graphemes in JS, but most of the default behaviour doesn't account for it so if we add these tests I can guarantee you that almost all the current solution will fail.
Do we expect students to be able to deal with such weirdness at the level that this exercise is? If we do end up adding the tests it'll probably be good to also amend the instructions with some sort of explanation. @SleeplessByte opinions?

---

I had to deal with this in micro-blog and i wrote up an entire Dig Deeper section about it, so maybe check that if you want to know more of the technical angle.

[SleeplessByte]

I haven't looked at the current proof. Is this updating from .split('') to [...x] / Array.from(x)?

If yes, we should do this. People shouldn't split into characters like the first one anyway. If no, and it is harder, we can decide between not including or making them optional. We have a special flag and syntax to allow for that.

[jagdish-15]

I'll look into it this Monday or Tuesday!

[jagdish-15]

I got it running after checking out the Intl.Segmenter object from the Dig Deeper section by @Cool-Katt!

Do we expect students to be able to deal with such weirdness at the level that this exercise is? If we do end up adding the tests it'll probably be good to also amend the instructions with some sort of explanation.

Now that's a topic of discussion. What do you think @SleeplessByte @Cool-Katt

[jagdish-15]

Since this is a practice exercise, I think it’s totally fine to include these tests. If we decide to go ahead, we’ll just need to add an instructions.append.md file and update the difficulty from Easy to Medium.

Alternatively, we can always choose to skip the tests.
Let me know what you think!

[SleeplessByte]

Can you try if let characters = [...string] works ?

[Cool-Katt]

Well, I checked the current solutions and I don't really know how many total submissions there are, but exactly two of them use Intl.Segmenter, and one of the two is kinda "cheety" by using .reverse() directly, so I can tell you that this change will cause some confusion.

This discussion may need to happen on the forum to get more voices on the matter, so feel free to open a thread and link this PR there @jagdish-15. Presonaly, I'm tentatively for a merge and bumping the difficulty.

[Cool-Katt]

@SleeplessByte If you mean something like this:

const expected = 'dnatsnehctsrüW';
const actual = 'Würstchenstand';
console.log([...actual].reverse().join('').localeCompare(expected))
// => 1 (not the same)

This definitely doesn't work, in the test with 'Würstchenstand', it puts the umlaut (the two dots on top of the 'u') on the 'r' instead, which is also frustrating because it visually is hard to tell the difference.

[SleeplessByte]

Ok perfect.

I dont think we should make this exercise more difficult. I rather have an exercise that goes further exploring this concept.

[Cool-Katt]

We do (kind of)! micro-blog is dealing with this weirdness.

I'm okay with not complicating things more for this exercise, as it is a more beginner friendly exercise.

[SleeplessByte]

We do (kind of)! micro-blog is dealing with this weirdness.

I'm okay with not complicating things more for this exercise, as it is a more beginner friendly exercise.

Ah yeah, I remember that one!

@jagdish-15 can you remove the tests that require the segmenter?

[jagdish-15]

Can you try if let characters = [...string] works ?

I did try it, but it didn't work, as @Cool-Katt showed

@jagdish-15 can you remove the tests that require the segmenter?

Should I remove it altogether or skip them using test.skip(...)?

[SleeplessByte]

Should I remove it altogether or skip them using test.skip(...)?

Oh right. We have that option. I guess we can add a track insert (instruction addendum):

~~~exercism/advanced
If you solve this using the CLI, there are test cases that require you to deal with complex characters.
You can optionally enable these tests by removing `.skip` from the test.
~~~

@Cool-Katt what do you think?

[jagdish-15]

I guess we can add a track insert (instruction addendum):

I vaguely remember seeing something similar already implemented in another exercise. In that case, comments were added above the skipped tests to inform CLI users that the tests are skipped by default (since most solutions would time out), but they can enable them manually if they want to experiment.

So I think staying consistent with that approach would be a good idea! But we'll see what @Cool-Katt says

[Cool-Katt]

Yep, I agree with skip and an addendum.

[jagdish-15]

I was discussing this exercise, pythagorean-triplet test file. It has skipped tests, but there is no addendum referring to that. Should we append instructions there as well, to address the skipped tests?

[Cool-Katt]

Yep.
People rarely ever read the test file separately so I feel that the note that DJ suggested is good to include.

[jagdish-15]

I've added the part suggested by @SleeplessByte and raised another PR (#2671) to do something similar for the reverse-string exercise. @Cool-Katt and @SleeplessByte take a look and lemme know if it works!

[SleeplessByte]

This one passes with the previous solution? If yes, this is good, if no it also needs to be skipped :)

[jagdish-15]

Yes it does indeed pass with the previous solution (I remember it did pass with [...string]) but I'll still check it once again to be sure

[jagdish-15]

Yes, the old solution passes the test as well, so there’s no need to skip it.
By the way, should we stick with the segmenter solution or revert to the original?
I think keeping the segmenter version is a better choice, especially since it also passes the skipped tests.

[Cool-Katt]

The proof solution is more for the CI, to prove that the tests can be passed, so the segmenter version can stay.

[SleeplessByte]

See one comment