Update anagram tests

[CadeMichael]

Followed the guide for updating testcases, using configlet to sync test.toml and making the updates in the testfile.

[vaeng]

Hey, thanks for the PR.

Do I understand correctly, that you updated the toml-file to reflect the current state of the tests and rewrote the tests to the format, that is outlined in the wiki?

If yes: would you be able to add the missing test cases as well?

[CadeMichael]

Hey, thanks for the PR.

Do I understand correctly, that you updated the toml-file to reflect the current state of the tests and rewrote the tests to the format, that is outlined in the wiki?

If yes: would you be able to add the missing test cases as well?

Yep! I got configlit to sync and update the toml and then made the changes. How do you mean missing tests? I took out the non included ones and added the new versions. Do you mean add the uuid's to the pre existing tests?

[IsaacG]

Yep! I got configlit to sync and update the toml and then made the changes. How do you mean missing tests? I took out the non included ones and added the new versions. Do you mean add the uuid's to the pre existing tests?

The TOML file is metadata about the unit tests. The unit tests list in the cpp test file. Any changes to the TOML files should have a corresponding change in the test.cpp file.

[CadeMichael]

Yep! I got configlit to sync and update the toml and then made the changes. How do you mean missing tests? I took out the non included ones and added the new versions. Do you mean add the uuid's to the pre existing tests?

The TOML file is metadata about the unit tests. The unit tests list in the cpp test file. Any changes to the TOML files should have a corresponding change in the test.cpp file.

Got it that's what I did, got the changes to sync then updated the cpp test file. Sorry for the imprecise wording I did no manual changes to the TOML I meant made changes to the cpp test file.

[vaeng]

I am not sure we mean the same thing.

Our goal for the track is to implement every test from the problem-specification as a single test case.
The problem-spec for anagram has 22 different test cases. The anagram_test.cpp has 16.

Unless there is a good reason to skip certain tests, this goal is not yet fulfilled for your implementation.
This could be a question of unicode representation ("fd3509e5-e3ba-409d-ac3d-a9ac84d13296") or capitalization of non-ascii letters ("a6854f66-eec1-4afd-a137-62ef2870c051").

In your test,cpp file some cases are missing their uuid.

[CadeMichael]

Gotcha so even when it says

[a0705568-628c-4b55-9798-82e4acde51ca]
description = "words other than themselves can be anagrams"
include = false

the test should be included. I assumed it meant include the newer version and skip the old version I'll put the old version back. I added the UUID for the new tests I'll put them in for the old tests as well.

[IsaacG]

include = false

If it says include = false this indicates that a particular test was explicitly excluded for some reason. Typically this occurs when the test doesn't make sense for a particular language for some reason. Or when a specific test is superseded ("reimplemented") by another test, but I'm not certain about that.

[CadeMichael]

should I put the tests back in that say 'include = false' ?

[IsaacG]

Looking at the canonical cases,

» jq -r '.cases[]|.uuid' canonical-data.json | grep -vf <( jq -r '.cases[]|select(.reimplements).reimplements' canonical-data.json)
dd40c4d2-3c8b-44e5-992a-f42b393ec373
03eb9bbe-8906-4ea0-84fa-ffe711b52c8b
a27558ee-9ba0-4552-96b1-ecf665b06556
64cd4584-fc15-4781-b633-3d814c4941a4
99c91beb-838f-4ccd-b123-935139917283
78487770-e258-4e1f-a646-8ece10950d90
1d0ab8aa-362f-49b7-9902-3d0c668d557b
9e632c0b-c0b1-4804-8cc1-e295dea6d8a8
b248e49f-0905-48d2-9c8d-bd02d8c3e392
f367325c-78ec-411c-be76-e79047f4bd54
630abb71-a94e-4715-8395-179ec1df9f91
9878a1c9-d6ea-4235-ae51-3ea2befd6842
68934ed0-010b-4ef9-857a-20c9012d1ebf
589384f3-4c8a-4e7d-9edc-51c3e5f0c90e
ba53e423-7e02-41ee-9ae2-71f91e6d18e6
33d3f67e-fbb9-49d3-a90e-0beb00861da7
a6854f66-eec1-4afd-a137-62ef2870c051
fd3509e5-e3ba-409d-ac3d-a9ac84d13296

That's the list of not-reimplemented test cases which ought to show up in the test file.

I noticed some of the TEST_CASE entries in the test file contain the UUID and others do not; if updating this file, it would be nice to add them to all TEST_CASE entries.

It looks like the test code in the PR does have all the expected test cases expect the last two which were recently added with "scenarios": ["unicode"]. Depending on whether or not the C++ would like to add Unicode to this exercise, those may or may not be wanted here. Ignoring those, I think the test file looks like the tests are all correctly synced. All the test cases appear to use the same data as the problem specs.

[IsaacG]

The problem-spec for anagram has 22 different test cases. The anagram_test.cpp has 16.

The problem spec has 22 test cases but 6 are reimplemented so only 16 ought to show up.

[CadeMichael]

I added the uuid's but not the 'include = false' tests I can add those if needed pls let me know.

[vaeng]

I added the uuid's but not the 'include = false' tests I can add those if needed pls let me know.

Yes, please include all the tests even the "false" ones if you can.

Edit: this might be wrong, please ignore for the time being.

[IsaacG]

Yes, please include all the tests even the "false" ones if you can.

In the tests.toml or in the C++ unit tests?

[vaeng]

Sorry, I need to look into the 'false'. all the way. I was not aware of the implementation.

We do need to discuss the unicode thing though.
@siebenschlaefer What is your opinion on the topic?

[CadeMichael]

got it added all tests

[IsaacG]

@vaeng Should this test be included in the C++ unit test file or not?
The canonical data says it's superseded and this line of tests.toml says it's not included, but the unit test file now includes it.

[vaeng]

@CadeMichael I am sorry for the confusion. I have not seen that some tests are not supposed to be included in the tests, as they are updated. I first thought you had added the include = false yourself so that the test.toml reflected the original state of the tests.

You were right not including them. Thanks for adding all the uuids.

For the failing tests, we have to update the catch version. At least I think that this is the issue. Unique names are not required for catch2, as long as the tags are different.

[vaeng]

Catch2 v3 changes run fine on my machine, but make problems in the CI.

I see two ways:

• fixing the student files and cmake entries to allow for v3 of Catch2. Or
• do not allow repeated names.

I have touched on the subject of the latter alternative with @siebenschlaefer in a private conversation.
In the short term we should do the second, in the long term we should also implement catch2 v3 into normal exercises. Currently the test-runner uses a new version, but the "prepackaged" version that is downloaded to user directories and used in the CI is not new.

[siebenschlaefer]

We do need to discuss the unicode thing though.
@siebenschlaefer What is your opinion on the topic?

Reversing a Unicode string correctly without a library is nearly impossible, unless you only want to handle some basic/simple cases.
Bethanyg did write a little bit about it on Slack and in a discussion here on GitHub.

We could include ICU in our docker image but I'd vote against it. Proper Unicode handling would make this a different exercise, and a much harder one.

[vaeng]

I also vote to not include unicode tests. The correct way would be to have an inlcude = false line for these, when I read the docs correctly?

[IsaacG]

I also vote to not include unicode tests. The correct way would be to have an inlcude = false line for these, when I read the docs correctly?

Yup. This would be where you'd set that manually.

[CadeMichael]

Alright I will remove the include false tests and the unicode tests and add include = false to those. The commit will be in within 5 min of this comment.

[vaeng]

Thanks for all the work!