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]

See one comment

[devcyjung]

@SleeplessByte This PR seems to be causing error in online editor

I suspect changing test.skip to xtest would fix it

Error message:

We received the following error when we ran your code:

Expected to see 0 skipped tests and 0 skipped test suites. None of the tests in this exercise are optional. The skipped tests will not show up, but were found during the last run.

[jagdish-15]

Yes, we do receive the error message, though it's unclear why. We use a similar test-skipping pattern in the pythagorean-triplet test file, and that one doesn’t seem to cause any problems.

I suspect changing test.skip to xtest would fix it.

That’s probably not the issue. We already use xtest for the regular cases as part of the test runner’s setup — only the first test is defined using test, and the rest rely on xtest by design.

@Cool-Katt @SleeplessByte, got any clue?

[devcyjung]

the way I see it is test.skip is for computationally heavy test cases that would take too long to complete in online environment, otherwise xtest is used

If you look at the scripts there are specific commands such as

sed -i -e 's/xtest/test/g' "${test_file}"

which replace xtest with test before running the tests

Also this is where the error is coming from

https://github.com/exercism/js-test-runner-clientside/blob/94e033dd93e9260b64dabe4ccef315d20a028b7a/src/output.ts#L40

You should either flag it as optional test or use the xtest @jagdish-15

[SleeplessByte]

It should use test.skip.

It's possible that we just got the new online runner. I just haven't enabled the online user to be able to handle this. If that's the case (I'll check) and because it takes a few days to make changes to the online editor, we'll comment out the test for now.

If that's not the issue the. It should still use test.skip and something else is going on.

[jagdish-15]

The pythagorean-triplet tests are working fine, even though they also use test.skip(), so the issue seems to be specific to our current case.

Still, should I go ahead and push the changes after fully commenting out the code?

[SleeplessByte]

The pythagorean-triplet tests are working fine, even though they also use test.skip(), so the issue seems to be specific to our current case.

Still, should I go ahead and push the changes after fully commenting out the code?

Nah, I am investigating now as you mentioned test.skip is fine.

[SleeplessByte]

@jagdish-15 the issue is flag.tests.includes-optional is not set to true in the config, which is what @devcyjung also mentioned in the last sentence. Can you PR that so I can merge it?

[jagdish-15]

@SleeplessByte
I've raised the PR #2682 (please discard the earlier one, sorry for that)