parallel-letter-frequency: Sync tests & add missing #2050

ellnix · 2025-03-03T19:32:56Z

The tests are unique and not really in sync with problem-specifications, however they test the same behavior so it should be fine? I thought I'd make a PR before making a template since it might make sense to keep the bespoke tests that we have now. Would like any input on this.

The tests I added were missing, but they are just convenience tests for students to be able to iterate their solution, so I think this is appropriate:

[no important files changed]

The tests are unique and not really in sync with problem-specifications, however they test the same behavior so it should be fine? The tests I added were missing, but they are just convenience tests for students to be able to iterate their solution, so I think this is appropriate: [no important files changed]

github-actions · 2025-03-03T19:33:13Z

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!

senekor · 2025-03-03T20:43:49Z

I don't really understand. Where are the three additional tests coming from? Do you propose a second PR later with a template?

In general, I'd be in favor of adopting the tests from problem-specifications after checking that the tested behavior is the same.

Making the TDD experience more granular for students doesn't seem like a compelling enough argument for me to introduce custom tests. Usually that makes sense if they are inherently Rust-specific or somehow needed to preserve backwards-compatibility with an existing exercise design.

ellnix · 2025-03-03T21:05:45Z

I don't really understand. Where are the three additional tests coming from? Do you propose a second PR later with a template?

I should have been clearer, those tests are in problem-specifications but not in Rust. I would probably just add a commit to this PR for the template, don't want to add too much noise to the repo.

This is the rename:

[818031be-49dc-4675-b2f9-c4047f638a2a]
description = "one text with one letter"

These are the new tests:

[c0b81d1b-940d-4cea-9f49-8445c69c17ae]
description = "one text with multiple letters"

[708ff1e0-f14a-43fd-adb5-e76750dcf108]
description = "two texts with one letter"

[1b5c28bb-4619-4c9d-8db9-a4bb9c3bdca0]
description = "two texts with multiple letters"

In general, I'd be in favor of adopting the tests from problem-specifications after checking that the tested behavior is the same.

Alright, so I'm assuming I should go ahead and make a template for this test?

senekor · 2025-03-03T21:06:46Z

Ah, got it. Yeah, a template sounds good.

ellnix · 2025-03-09T15:46:12Z

@senekor I attempted this a couple days ago and I don't honestly think a template is worth it, there's tests with very long text, and also a test with repeating text which would add like 30 lines alone.

Test with repeating text

{
      "uuid": "adf8e57b-8e54-4483-b6b8-8b32c115884c",
      "description": "many small texts",
      "scenarios": ["concurrent"],
      "property": "calculateFrequencies",
      "input": {
        "texts": [
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc",
          "abbccc"
        ]
      },
      "expected": {
        "a": 50,
        "b": 100,
        "c": 150
      }
    }

Keeping the text from being long and unwieldy would require as much code in Tera as there is in the test file itself. I think the way we have it with the texts hoisted at the top makes the most sense.

senekor · 2025-03-09T16:01:58Z

That's ok, but in that case we need to make sure that tests.toml actually matches our manually-maintained tests. For example, tests.toml currently contains the following:

[adf8e57b-8e54-4483-b6b8-8b32c115884c]
description = "many small texts"

That's the unwieldy test you posted. If we don't include this test, then it must be marked as excluded with a justification in tests.toml.

One advantage the templates give us is that they automatically make sure all the tests that are declared as included in tests.toml are in fact included.

If we're gonna maintain the mapping manually, we should add a comment above every test with its UUID or something so we can easily trace that work in the future.

I'm open to alternatives, but what we shouldn't do is add a tests.toml file that contains incorrect information about which tests are actually implemented and which aren't.

senekor

so you can re-request a review :)

senekor

I meant re-request when you think I should take another look at the code 😄

ellnix · 2025-03-09T16:19:00Z

Oh lmao, I thought for some reason you wanted to go through the request-response workflow again for some reason, my bad. I did a git reset --hard on that attempt to add the template because I felt like I wasn't getting anywhere. I got a notification from github about your request for changes but not the first message you sent, and it scrolled past it 😫

I'm open to alternatives, but what we shouldn't do is add a tests.toml file that contains incorrect information about which tests are actually implemented and which aren't.

That test is actually present, and it's just a call to vec! instead of all those lines.

#[test]
#[ignore]
fn many_times_same_text() {
    let v = vec!["abc"; 1000];
    let mut hm = HashMap::new();
    hm.insert('a', 1000);
    hm.insert('b', 1000);
    hm.insert('c', 1000);
    assert_eq!(frequency::frequency(&v[..], 4), hm);
}

If we're gonna maintain the mapping manually, we should add a comment above every test with its UUID or something so we can easily trace that work in the future.

I think this would be really confusing since students see those comments. Since tests.toml should keep all arbitrary pairs, we can add a rust_fn = many_times_same_text key or similar. That would also be much easier to parse in CI, wherein we could try to match every entry in tests.toml with a function matching the description or rust_fn.

senekor · 2025-03-09T16:29:06Z

Since tests.toml should keep all arbitrary pairs, we can add a rust_fn = many_times_same_text key or similar.

Great idea!

That would also be much easier to parse in CI, wherein we could try to match every entry in tests.toml with a function matching the description or rust_fn.

Also a great idea! But I'm not gonna block this PR on it. If you want to do it, go ahead, otherwise feel free to leave it for later. My main concern is that I don't want maintainers to have to do the manual work of matching up tests.toml with the actual tests over and over. With the addtional key in tests.toml, that's pretty much covered for me.

Used a vec to keep the tests consistent, but clippy disagrees.

ellnix · 2025-03-09T21:44:09Z

exercises/practice/parallel-letter-frequency/.meta/tests.toml

+[7b1da046-701b-41fc-813e-dcfb5ee51813]
+description = "combination of lower- and uppercase letters, punctuation and white space"
+rust_fn = "all_three_anthems_1_worker"


I felt it was appropriate to match tests based on the behavior under test, not necessarily the exact content of the input.

For reference, the input of "combination of lower- and uppercase letters, punctuation and white space" is this:

"input": { "texts": [ "There, peeping among the cloud-wrack above a dark tower high up in the mountains, Sam saw a white star twinkle for a while. The beauty of it smote his heart, as he looked up out of the forsaken land, and hope returned to him. For like a shaft, clear and cold, the thought pierced him that in the end, the shadow was only a small and passing thing: there was light and high beauty forever beyond its reach." ] },

I agree. If we're maintaining the tests manually, we might as well grant ourselves the flexibility a template would lack. Behavior is what counts.

ellnix · 2025-03-09T21:50:46Z

exercises/practice/parallel-letter-frequency/tests/parallel_letter_frequency.rs

@@ -129,3 +177,183 @@ fn non_integer_multiple_of_threads() {
    hm.insert('c', 999);
    assert_eq!(frequency::frequency(&v[..], 4), hm);
 }
+
+const DOSTOEVSKY: [&str; 4] = [


This text is really large so I thought I'd have it after all the tests that work without it so the file is easier to skim. I can understand the argument for having all of the constants defined at the top though.

I don't feel strongly in either case.

What about putting all the texts at the very bottom below the last test? That seems consistent and most conducive to skimming the file. If users want to read the actual texts to understand a test case, they will have to move around the file one way or the other.

I like the idea, since they are constants anyway we have that flexibility.

Implemented: 4d0bcfc

senekor · 2025-03-09T21:59:35Z

exercises/practice/parallel-letter-frequency/tests/parallel_letter_frequency.rs

@@ -129,3 +177,183 @@ fn non_integer_multiple_of_threads() {
    hm.insert('c', 999);
    assert_eq!(frequency::frequency(&v[..], 4), hm);
 }
+
+const DOSTOEVSKY: [&str; 4] = [


What about putting all the texts at the very bottom below the last test? That seems consistent and most conducive to skimming the file. If users want to read the actual texts to understand a test case, they will have to move around the file one way or the other.

exercises/practice/parallel-letter-frequency/.meta/tests.toml

senekor · 2025-03-09T22:04:26Z

exercises/practice/parallel-letter-frequency/.meta/tests.toml

+[7b1da046-701b-41fc-813e-dcfb5ee51813]
+description = "combination of lower- and uppercase letters, punctuation and white space"
+rust_fn = "all_three_anthems_1_worker"


I agree. If we're maintaining the tests manually, we might as well grant ourselves the flexibility a template would lack. Behavior is what counts.

senekor

Thanks!

ellnix requested a review from senekor March 3, 2025 19:32

senekor requested changes Mar 9, 2025

View reviewed changes

ellnix requested a review from senekor March 9, 2025 16:06

senekor requested changes Mar 9, 2025

View reviewed changes

ellnix added 7 commits March 9, 2025 22:25

parallel-letter-frequency: Rename tests to match problem-specifications

35ca8ca

parallel-letter-frequency: Add test function names

a73b6f6

parallel-letter-frequency: Add missing tests

790d0fc

parallel-letter-frequency: Fix typo in test

a8da3f1

parallel-letter-frequency: Replace vec with array

7939dbd

Used a vec to keep the tests consistent, but clippy disagrees.

parallel-letter-frequency: Add "Unicode letters" test

9b3791e

parallel-letter-frequency: Run rustfmt

50d0072

ellnix commented Mar 9, 2025

View reviewed changes

ellnix requested a review from senekor March 9, 2025 21:54

senekor reviewed Mar 9, 2025

View reviewed changes

senekor mentioned this pull request Mar 9, 2025

Add CI test for manually maintained tests.toml files #2059

Open

parallel-letter-frequency: Move inputs under tests

4d0bcfc

senekor approved these changes Mar 9, 2025

View reviewed changes

ellnix merged commit 4bbbdeb into exercism:main Mar 9, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parallel-letter-frequency: Sync tests & add missing #2050

parallel-letter-frequency: Sync tests & add missing #2050

ellnix commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

senekor commented Mar 3, 2025

ellnix commented Mar 3, 2025

senekor commented Mar 3, 2025

ellnix commented Mar 9, 2025

senekor commented Mar 9, 2025

senekor left a comment

senekor left a comment

ellnix commented Mar 9, 2025

senekor commented Mar 9, 2025

ellnix Mar 9, 2025

ellnix Mar 9, 2025

senekor Mar 9, 2025

ellnix Mar 9, 2025

senekor Mar 9, 2025

ellnix Mar 9, 2025

ellnix Mar 9, 2025 •

edited

Loading

senekor Mar 9, 2025

senekor Mar 9, 2025

senekor left a comment

parallel-letter-frequency: Sync tests & add missing #2050

parallel-letter-frequency: Sync tests & add missing #2050

Conversation

ellnix commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

senekor commented Mar 3, 2025

ellnix commented Mar 3, 2025

senekor commented Mar 3, 2025

ellnix commented Mar 9, 2025

senekor commented Mar 9, 2025

senekor left a comment

Choose a reason for hiding this comment

senekor left a comment

Choose a reason for hiding this comment

ellnix commented Mar 9, 2025

senekor commented Mar 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellnix Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senekor left a comment

Choose a reason for hiding this comment

ellnix Mar 9, 2025 •

edited

Loading