sync the largest-series-product tests and fix / add missing unit tests #3051

xinri · 2025-11-16T22:48:01Z

pull request

Sync the largest-series-product. Missing reimplement (re-generated with configlet)

related to #2959

Reviewer Resources:

…but not sync

jagdish-15

Also check if there are any other tests that are marked as include = false but still exist in the test file. And please make all the error messages consistent with those in the canonical-data.json file. I’ve suggested changes for one of them, but I believe there are more cases like that, for example the rejects negative span test.

jagdish-15 · 2025-11-18T07:14:43Z

exercises/practice/largest-series-product/src/test/java/LargestSeriesProductCalculatorTest.java


        assertThatExceptionOfType(IllegalArgumentException.class)
                .isThrownBy(() -> calculator.calculateLargestProductForSeriesLength(4))
                .withMessage("Series length must be less than or equal to the length of the string to search.");


Suggested change

.withMessage("span must not exceed string length");

Well it is indeed what the canonical said but I read that the implementation of the error messages should be made easy to read by the devs. I checked a lot of unit tests and their canonical file, and they have their own way to express what the canonical file raised as an error. So we should not just copy paste the error message but we should create an explicit message that will represent the error message in the canonical file no?

Indeed, the message in the specification is not representing the error message.
I have replace it with "Series length must not exceed the length of the string to search." to match with the function.

By the way, if some mentee works on this exercise, they will have to rework again no ?

I'd prefer to leave the exception messages as they were. I think your wording is saying the same thing as before, just with different wording.

By the way, if some mentee works on this exercise, they will have to rework again no ?

Yeah, if we change the wording, the students will need to update their solutions. Since the wording is saying the same thing, I don't think there is much benefit with changing the exception message (hence, my preference for leaving the exception message as is).

sorry I didn't see this answer. Thanks for the confirmation. I also prefer to keep the same error message because they are quite explicits

jagdish-15 · 2025-11-18T07:24:41Z

exercises/practice/largest-series-product/src/test/java/LargestSeriesProductCalculatorTest.java

    @Disabled("Remove to run test")
    @Test
    @DisplayName("reports 1 for empty string and empty product (0 span)")
-    public void testEmptyStringToSearchAndSeriesOfNonZeroLengthIsRejected() {
+    public void testEmptyStringToSearchAndEmptyProduct() {
+        LargestSeriesProductCalculator calculator = new LargestSeriesProductCalculator("");
+
+        long actualProduct = calculator.calculateLargestProductForSeriesLength(0);
+
+        assertThat(actualProduct).isEqualTo(1);
+    }
+
+    @Disabled("Remove to run test")
+    @Test
+    @DisplayName("reports 1 for nonempty string and empty product (0 span)")
+    public void testNonEmptyStringToSearchAndEmptyProduct() {
+        LargestSeriesProductCalculator calculator = new LargestSeriesProductCalculator("123");
+
+        long actualProduct = calculator.calculateLargestProductForSeriesLength(0);
+
+        assertThat(actualProduct).isEqualTo(1);
+    }


Remove the tests which are marked as include = false in the toml file

done thanks for checking that

kahgoh · 2025-11-18T22:12:35Z

exercises/practice/largest-series-product/src/test/java/LargestSeriesProductCalculatorTest.java

        assertThatExceptionOfType(IllegalArgumentException.class)
                .isThrownBy(() -> calculator.calculateLargestProductForSeriesLength(1))
-                .withMessage("Series length must be less than or equal to the length of the string to search.");
+                .withMessage("Series length must not exceed the length of the string to search.");


As mentioned in another comment, let's keep the message as it was.

ok I have done a rollback for this

kahgoh · 2025-11-18T22:12:40Z

exercises/practice/largest-series-product/src/test/java/LargestSeriesProductCalculatorTest.java

        assertThatExceptionOfType(IllegalArgumentException.class)
                .isThrownBy(() -> calculator.calculateLargestProductForSeriesLength(-1))
-                .withMessage("Series length must be non-negative.");
+                .withMessage("Series length must not be negative.");


As mentioned in another comment, let's keep the message as it was.

ok I have done a rollback for this

Signed-off-by: xinri <[email protected]>

sync the largest-series-product tests as they had been reimplemented …

2a74ff4

…but not sync

xinri marked this pull request as draft November 16, 2025 23:18

xinri marked this pull request as ready for review November 17, 2025 21:45

xinri force-pushed the sync_largest_series_product_tests branch from 772b252 to 33c41e2 Compare November 17, 2025 22:00

xinri changed the title ~~sync the largest-series-product tests as they had been reimplemented but not sync~~ sync the largest-series-product tests and fix / add missing unit tests Nov 17, 2025

xinri force-pushed the sync_largest_series_product_tests branch 2 times, most recently from 0465015 to bcfa8e6 Compare November 17, 2025 23:03

jagdish-15 requested changes Nov 18, 2025

View reviewed changes

xinri force-pushed the sync_largest_series_product_tests branch 3 times, most recently from 903baf0 to 34baef8 Compare November 18, 2025 18:32

xinri requested a review from jagdish-15 November 18, 2025 18:32

kahgoh reviewed Nov 18, 2025

View reviewed changes

reimplement and fix unit tests in the largest series product feature

989dcfa

Signed-off-by: xinri <[email protected]>

xinri force-pushed the sync_largest_series_product_tests branch from 34baef8 to 989dcfa Compare November 18, 2025 23:18

xinri requested a review from kahgoh November 18, 2025 23:26

Merge branch 'main' into sync_largest_series_product_tests

0bc9d60

jagdish-15 approved these changes Nov 19, 2025

View reviewed changes

jagdish-15 added the x:rep/small Small amount of reputation label Nov 19, 2025

kahgoh approved these changes Nov 19, 2025

View reviewed changes

jagdish-15 merged commit 8419c95 into exercism:main Nov 19, 2025
6 checks passed

Uh oh!

sync the largest-series-product tests and fix / add missing unit tests #3051

sync the largest-series-product tests and fix / add missing unit tests #3051

Uh oh!

Conversation

xinri commented Nov 16, 2025

pull request

Uh oh!

jagdish-15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kahgoh Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kahgoh Nov 18, 2025 •

edited

Loading