-
-
Notifications
You must be signed in to change notification settings - Fork 744
Issue #2971 display names #3055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #2971 display names #3055
Conversation
|
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. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you apply these small changes:
| import io.reactivex.disposables.Disposable; | ||
| import org.junit.jupiter.api.BeforeEach; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import io.reactivex.disposables.Disposable; | |
| import org.junit.jupiter.api.BeforeEach; | |
| import io.reactivex.disposables.Disposable; | |
| import org.junit.jupiter.api.BeforeEach; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| import org.junit.jupiter.api.Disabled; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.DisplayName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import org.junit.jupiter.api.Disabled; | |
| import org.junit.jupiter.api.Test; | |
| import org.junit.jupiter.api.DisplayName; | |
| import org.junit.jupiter.api.Disabled; | |
| import org.junit.jupiter.api.DisplayName; | |
| import org.junit.jupiter.api.Test; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kahgoh, I need your suggestion on a few of these:
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Ability modifier for score 18 is 4") | ||
| public void testAbilityModifierForScore18Is4() { | ||
| assertThat(dndCharacter.modifier(18)).isEqualTo(4); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please keep all of them the same as the descriptions in the canonical-data.json file?
And something to discuss, @kahgoh: only the tests up to this one are fully in sync with the canonical data. There are three more tests after this: random character is valid, random ability is within range, and each ability is only calculated once. But there are several extra tests in this file that are not present in the canonical data. Do you think we should keep them or make this file consistent with the canonical data?
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a checked exception (not a RuntimeException)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kahgoh
This exercise doesn't have a canonical-data.json file, and I think we can go ahead with these descriptions. Do you have any nits for these?
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Initial game state is set correctly for a new word") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same goes with this one!
@kahgoh Do you have any nits for the descriptions?
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Plain text is wrapped in a paragraph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manjarekarsudip
All these should be in line with the canonical-data.json file
| public class BracketCheckerTest { | ||
|
|
||
| @Test | ||
| @DisplayName("Paired square brackets are matched and valid") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public class NaturalNumberTest { | ||
|
|
||
| @Test | ||
| @DisplayName("Small perfect number (6) is classified as PERFECT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public class PhoneNumberTest { | ||
|
|
||
| @Test | ||
| @DisplayName("Cleans common formatted number to digits only") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private static final double DOUBLE_EQUALITY_TOLERANCE = 1e-9; | ||
|
|
||
| @Test | ||
| @DisplayName("Complete information from pieces and aspect ratio is calculated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| public class PokerTest { | ||
| @Test | ||
| @DisplayName("Single hand returns itself as best hand") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Generated maze with seed is perfect (single path, no isolated cells)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this one seems to be a Java track-specific exercise, so it has no mention in the problem-specifications repo. I think we can go ahead with this one. Let's see what @kahgoh says about this
|
Is any other change required in any file? @jagdish-15 |
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the exercises that don’t have a canonical-data.json file, I’ll need to give detailed changes for each of them. And for all the exercises that do have a canonical-data.json file, here’s the only change required:
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("many small texts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace this test and the one below it with each other, so the order is the same as that in the canonical data
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done for the error-handling exercise. Now the descriptions will be more consistent with each other and follow a similar (standardised) format like the canonical data
| private ErrorHandling errorHandling = new ErrorHandling(); | ||
|
|
||
| @Test | ||
| @DisplayName("Throws a general exception when requested") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a general exception when requested") | |
| @DisplayName("Throws IllegalArgumentException") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a checked exception (not a RuntimeException)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a checked exception (not a RuntimeException)") | |
| @DisplayName("Throws any checked exception") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a checked exception with the provided detail message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a checked exception with the provided detail message") | |
| @DisplayName("Throws any checked exception with provided detail message") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws an unchecked RuntimeException when requested") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws an unchecked RuntimeException when requested") | |
| @DisplayName("Throws any unchecked exception") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws an unchecked RuntimeException with provided detail message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws an unchecked RuntimeException with provided detail message") | |
| @DisplayName("Throws any unchecked exception with provided detail message") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a custom checked exception") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a custom checked exception") | |
| @DisplayName("Throws custom checked exception") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a custom checked exception with provided message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a custom checked exception with provided message") | |
| @DisplayName("Throws custom checked exception with provided detail message") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a custom unchecked exception") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a custom unchecked exception") | |
| @DisplayName("Throws custom unchecked exception") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws a custom unchecked exception with provided message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws a custom unchecked exception with provided message") | |
| @DisplayName("Throws custom unchecked exception with provided detail message") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Returns Optional<Integer> for valid input and empty Optional for invalid input") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Returns Optional<Integer> for valid input and empty Optional for invalid input") | |
| @DisplayName("Handles error by throwing Optional instance") |
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn’t have many changes. The few that are there are to make the descriptions a bit more compact
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Cannot play the same correct guess twice") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Cannot play the same correct guess twice") | |
| @DisplayName("Cannot play the same guess twice") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Losing the game results in LOSS status and all parts present") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Losing the game results in LOSS status and all parts present") | |
| @DisplayName("Losing the game results in LOSS status") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Winning the game reveals full word and marks WIN status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Winning the game reveals full word and marks WIN status") | |
| @DisplayName("Winning the game results in WIN status") |
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Initial game state is set correctly for a new word") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Initial game state is set correctly for a new word") | |
| @DisplayName("Initial game state is set correctly") |
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After these changes, you’ll notice some test descriptions start with Maze, some with Mazes, and some with Throws. Please move the two perfect checking tests whose descriptions start with Maze above the ones whose start with Mazes, so the groups are ordered cleanly.
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Maze has a single entrance on the left side") | ||
| public void theMazeHasOnlyOneEntranceOnTheLeftSide() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void theMazeHasOnlyOneEntranceOnTheLeftSide() { | |
| public void theMazeHasSingleEntranceOnTheLeftSide() { |
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Maze has a single exit on the right side") | ||
| public void theMazeHasSingleExitOnTheRightSideOfTheMaze() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void theMazeHasSingleExitOnTheRightSideOfTheMaze() { | |
| public void theMazeHasSingleExitOnTheRightSide() { |
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Generated maze has correct overall dimensions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Generated maze has correct overall dimensions") | |
| @DisplayName("Maze has correct overall dimensions") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Maze contains only allowed characters") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Maze contains only allowed characters") | |
| @DisplayName("Maze contains only valid characters") |
Let's keep the keyword the same in the test name and the description
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Consecutive maze generations produce different mazes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Consecutive maze generations produce different mazes") | |
| @DisplayName("Maze is different each time it is generated") |
Let's try to keep it consistent with the others
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Generated maze with seed is perfect (single path, no isolated cells)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Generated maze with seed is perfect (single path, no isolated cells)") | |
| @DisplayName("Maze with a seed is generated perfectly (single path, no isolated cells)") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws when rows less than minimum allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws when rows less than minimum allowed") | |
| @DisplayName("Throws when rows are less than five") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws when columns less than minimum allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws when columns less than minimum allowed") | |
| @DisplayName("Throws when columns are less than five") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws when rows exceed maximum allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws when rows exceed maximum allowed") | |
| @DisplayName("Throws when rows exceed hundred") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Throws when columns exceed maximum allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Throws when columns exceed maximum allowed") | |
| @DisplayName("Throws when columns exceed hundred") |
jagdish-15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits:
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Robot names are unique in a large sample") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Robot names are unique in a large sample") | |
| @DisplayName("Robot names are unique") |
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Robot has a valid name matching the pattern") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Robot has a valid name matching the pattern") | |
| @DisplayName("Robot has a valid name") |
| public class SimpleLinkedListTest { | ||
|
|
||
| @Test | ||
| @DisplayName("A new list is empty with size zero") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("A new list is empty with size zero") | |
| @DisplayName("A new list is empty") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Create list from array sets correct size") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Create list from array sets correct size") | |
| @DisplayName("Create list from array") |
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("Convert list to array returns correct element order") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @DisplayName("Convert list to array returns correct element order") | |
| @DisplayName("Can return list as an array") |
exercises/practice/simple-linked-list/src/test/java/SimpleLinkedListTest.java
Outdated
Show resolved
Hide resolved
|
Just a general tip: instead of clicking |
|
Looks good to me, thanks! |
Welcome |
Pull request
Description
Added @DisplayName annotations to test methods based on canonical data
Changes
Added DisplayName annotations to the following exercises:
Testing
All tests pass successfully (verified with ./gradlew test)
Related Issue
Addresses #2971 (DisplayName annotations initiative)
Note
Below 2 exercises are the ones that i observed have the changes done as per the requirement but the checkbox is bot ticked.
Reviewer Resources:
Track Policies