Skip to content
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.

Sign up for GitHub

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

Robot simulator descriptions too long #1525

Merged
merged 4 commits into from
Jun 12, 2019
Merged

Robot simulator descriptions too long #1525

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea538f6
Robot simulator descriptions too long
bencoman Jun 5, 2019
12a2ccc
fix missing comma, thx @SleeplessByte
bencoman Jun 5, 2019
99706b8
update per suggestions from SleeplessByte
bencoman Jun 5, 2019
8cf9021
make descriptions a bit more verbose
bencoman Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 48 additions & 23 deletions exercises/robot-simulator/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
],
"cases": [
{
"description": "A robot is created with a position and a direction",
"description": "Create",
"comments": [ "A robot is created with a position and a direction" ],
"cases": [
{
"description": "Robots are created with a position and direction",
"description": "at origin facing north",
"comments": [ "Robots are created with a position and direction" ],
"property": "create",
"input": {
"position": {
Expand All @@ -31,7 +33,8 @@
}
},
{
"description": "Negative positions are allowed",
"description": "at negative position facing South",
"comments": [ "Negative positions allowed" ],
Copy link
Contributor

@SaschaMann SaschaMann Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment just repeats what the description says, so it could be removed. Same with the comments in L58, L62, L82 etc.

(I'm opposed to the suggested changes in general (see the other comment thread), but if this would go through, the redundancy should be removed imo)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the comment is superfluous here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same when I did it. I was just trying to not to lose any info. I'll wait to see how the other discussion pans out be doing such cleanup.

"property": "create",
"input": {
"position": {
Expand All @@ -51,10 +54,12 @@
]
},
{
"description": "rotates the robot's direction 90 degrees clockwise",
"description": "Rotate clockwise",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Rotate clockwise",
"description": "Rotate 90 degrees clockwise",

Copy link
Contributor Author

@bencoman bencoman Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to rotate other than 90 degrees? If not, it is superfluous to say so.
Maybe "Turn clockwise" would be better.

"comments": [ "rotates the robot's direction 90 degrees clockwise" ],
"cases": [
{
"description": "changes the direction from north to east",
"description": "north to east",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "north to east",
"description": "Change direction from north to east",

Copy link
Contributor Author

@bencoman bencoman Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know it changes direction, since the parent is "Rotate clockwise"
For a succinct unit test identifier, I feel the [1.] below doesn't lose any information
over @sshine suggestion [2.] and original [3.]

1. rotate_clockwise_north_to_east
2. rotate_90_degrees_clockwise_change_direction_from_north_to_east
3. rotates_the_robot's_direction_90_degrees_clockwise_changes_the_direction_from_north_to_east

"comments": [ "changes the direction from north to east" ],
"property": "move",
"input": {
"position": {
Expand All @@ -73,7 +78,8 @@
}
},
{
"description": "changes the direction from east to south",
"description": "east to south",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "east to south",
"description": "Change direction from east to south",

"comments": [ "changes the direction from east to south" ],
"property": "move",
"input": {
"position": {
Expand All @@ -92,7 +98,8 @@
}
},
{
"description": "changes the direction from south to west",
"description": "south to west",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "south to west",
"description": "Change direction from south to west",

"comments": [ "changes the direction from south to west" ],
"property": "move",
"input": {
"position": {
Expand All @@ -111,7 +118,8 @@
}
},
{
"description": "changes the direction from west to north",
"description": "west to north",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "west to north",
"description": "Change direction from west to north",

"comments": [ "changes the direction from west to north" ],
"property": "move",
"input": {
"position": {
Expand All @@ -132,10 +140,12 @@
]
},
{
"description": "rotates the robot's direction 90 degrees counter-clockwise",
"description": "Rotates counter-clockwise",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Rotates counter-clockwise",
"description": "Rotate 90 degrees counter-clockwise",

"comments": [ "rotates the robot's direction 90 degrees counter-clockwise" ],
"cases": [
{
"description": "changes the direction from north to west",
"description": "north to west",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "north to west",
"description": "Change direction from north to west",

"comments": [ "changes the direction from north to west" ],
"property": "move",
"input": {
"position": {
Expand All @@ -154,7 +164,8 @@
}
},
{
"description": "changes the direction from west to south",
"description": "west to south",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "west to south",
"description": "Change direction from west to south",

"comments": [ "changes the direction from west to south" ],
"property": "move",
"input": {
"position": {
Expand All @@ -173,7 +184,8 @@
}
},
{
"description": "changes the direction from south to east",
"description": "south to east",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "south to east",
"description": "Change direction from south to east",

"comments": [ "changes the direction from south to east" ],
"property": "move",
"input": {
"position": {
Expand All @@ -192,7 +204,8 @@
}
},
{
"description": "changes the direction from east to north",
"description": "east to north",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "east to north",
"description": "Change direction from east to north",

"comments": [ "changes the direction from east to north" ],
"property": "move",
"input": {
"position": {
Expand All @@ -213,10 +226,12 @@
]
},
{
"description": "moves the robot forward 1 space in the direction it is pointing",
"description": "Forward One",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Forward One",
"description": "Move forward one space in its own direction",

"comments": [ "moves the robot forward 1 space in the direction it is pointing" ],
"cases": [
{
"description": "increases the y coordinate one when facing north",
"description": "facing north",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "facing north",
"description": "Move forward facing north increases y coordinate",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats the same length as the original, which is pointless for me. The goal is facilitate reasonable length unit test names. And we already know its moving forward from the parent

"comments": [ "increases the y coordinate one when facing north" ],
"property": "move",
"input": {
"position": {
Expand All @@ -235,7 +250,8 @@
}
},
{
"description": "decreases the y coordinate by one when facing south",
"description": "facing south",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "facing south",
"description": "Move forward facing south decreases y coordinate",

"comments": [ "decreases the y coordinate by one when facing south" ],
"property": "move",
"input": {
"position": {
Expand All @@ -254,7 +270,8 @@
}
},
{
"description": "increases the x coordinate by one when facing east",
"description": "facing east",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "facing east",
"description": "Move forward facing east increases x coordinate",

"comments": [ "increases the x coordinate by one when facing east" ],
"property": "move",
"input": {
"position": {
Expand All @@ -273,7 +290,8 @@
}
},
{
"description": "decreases the x coordinate by one when facing west",
"description": "facing west",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "facing west",
"description": "Move forward facing west decreases x coordinate",

"comments": [ "decreases the x coordinate by one when facing west" ],
"property": "move",
"input": {
"position": {
Expand All @@ -294,10 +312,14 @@
]
},
{
"description": "Where R = Turn Right, L = Turn Left and A = Advance, the robot can follow a series of instructions and end up with the correct position and direction",
"description": "Series of instructions",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Series of instructions",
"description": "Follow Left, Right and Advance instructions",

This seems like a fine movement from description to comment.

This means that this comment most likely does not end up with the user.

Comments are meant for track maintainers / test generator writers.

So maybe this means that a cryptic part of an exercise becomes more cryptic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other instructions beside Left, Right, Advance? Otherwise its redundant to mention them.

"comments": [ "The robot can follow a series of instructions and end up with the correct position and direction.",
"Where R = Turn Right, L = Turn Left and A = Advance"
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing ,

Copy link
Contributor Author

@bencoman bencoman Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review. Some background....
Pharo generates individual method names for each unit test to be unique and fully descriptive by cascading the layers of descriptions. This works for all but a few exercises like robot-simulator, for which methods names arise as follows...

test01_ARobotIsCreatedWithAPositionAndADirectionRobotsAreCreatedWithAPositionAndDirection
test02_ARobotIsCreatedWithAPositionAndADirectionNegativePositionsAreAllowed
test03_RotatesTheRobotsDirection90DegreesClockwiseChangesTheDirectionFromNorthToEast
test04_RotatesTheRobotsDirection90DegreesClockwiseChangesTheDirectionFromEastToSouth
test05_RotatesTheRobotsDirection90DegreesClockwiseChangesTheDirectionFromSouthToWest
test06_RotatesTheRobotsDirection90DegreesClockwiseChangesTheDirectionFromWestToNorth
test07_RotatesTheRobotsDirection90DegreesCounterclockwiseChangesTheDirectionFromNorthToWest
test08_RotatesTheRobotsDirection90DegreesCounterclockwiseChangesTheDirectionFromWestToSouth
test09_RotatesTheRobotsDirection90DegreesCounterclockwiseChangesTheDirectionFromSouthToEast
test10_RotatesTheRobotsDirection90DegreesCounterclockwiseChangesTheDirectionFromEastToNorth
test11_MovesTheRobotForward1SpaceInTheDirectionItIsPointingIncreasesTheYCoordinateOneWhenFacingNorth
test12_MovesTheRobotForward1SpaceInTheDirectionItIsPointingDecreasesTheYCoordinateByOneWhenFacingSouth
test13_MovesTheRobotForward1SpaceInTheDirectionItIsPointingIncreasesTheXCoordinateByOneWhenFacingEast
test14_MovesTheRobotForward1SpaceInTheDirectionItIsPointingDecreasesTheXCoordinateByOneWhenFacingWest
test15_WhereRTurnRightLTurnLeftAndAAdvanceTheRobotCanFollowASeriesOfInstructionsAndEndUpWithTheCorrectPositionAndDirectionInstructionsToMoveEastAndNorthFromREADME
test16_WhereRTurnRightLTurnLeftAndAAdvanceTheRobotCanFollowASeriesOfInstructionsAndEndUpWithTheCorrectPositionAndDirectionInstructionsToMoveWestAndNorth
test17_WhereRTurnRightLTurnLeftAndAAdvanceTheRobotCanFollowASeriesOfInstructionsAndEndUpWithTheCorrectPositionAndDirectionInstructionsToMoveWestAndSouth
test18_WhereRTurnRightLTurnLeftAndAAdvanceTheRobotCanFollowASeriesOfInstructionsAndEndUpWithTheCorrectPositionAndDirectionInstructionsToMoveEastAndNorth

which is a bit awful to ask a GUI IDE to deal with.

The canonical schema specifies both "description" and "comments". One of those is implicitly longer, and the other needs to provide a minimal identifier for the test. What other data is available to generate unit test method names of reasonable length?
btw, How are method names generated for other languages?

I didn't spend the time making this edit to shorten the descriptions only for the sake of making them shorter. Its about distinguishing a descriptive-identifier from a verbose-comment.
I'd previously suggested in #1473 adding a separate field for this, but the preferred path given to me was shortening the "description" field. I'd be interested in review from @rpottsoh @ErikSchierboom.

Copy link
Contributor Author

@bencoman bencoman Jun 5, 2019 •
edited by SleeplessByte
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single word or 3 word descriptions. You lose a lot of information in the test suite this way.

That is what comments are for.
But I appreciate that there is a random sampling of approaches among language tracks, and curious on the details of how "comments" are handled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is a bit awful to ask a GUI IDE to deal with.

Oof. I can imagine. That looks like quite a bit 😓. Since the names are pretty descriptive in their original form, would this work by not cascading them?

and curious on the details of how "comments" are handled.

Comments are for the generator / implementer, free form and no rules.

Copy link
Contributor

@SaschaMann SaschaMann Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are method names generated for other languages?

#1411 has some info on that. To add to it: If we look at Python, Go and C# implementations of this exercise, only C# uses the description for method naming. Perhaps you could take a look at Go or Python for other options.
In the WIP Julia generator I'm working on the description is added as a comment and the comment is discarded but the idea of naming (or describing) every single test is not a thing in the language anyway.

That is what comments are for.

That's what the description is for. From the README:

Keep in mind that the description should not simply explain what each case is (that is redundant information) but also why each case is there. For example, what kinds of implementation mistakes might this case help us find?

I don't actually think there was anything wrong with any of these descriptions.

I do think they don't do a great job of explaining the why but these changes don't help with that issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think they don't do a great job of explaining the why but these changes don't help with that issue.

Right. What I meant that I didn't think the length was problematic as is ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Python & Go don't use directly use the description, my changes will have no impact on them. Can you provide some concrete code examples of where my changes cause harm?

Reviewing Python robot_simulator_test.py
and Go robot_simulator_test.go robot_simulator_step2_test.go & robot_simulator_step3_test.go
their method names look like they aren't generated and so don't directly depend on the descriptions. So I don't understand what "other options" I can infer from them.

Reviewing C#, it looks like their method names are generated similar to Pharo by concatenating each level's description. Their longest is an 190 character identifier. This change may be beneficial to them? @robkeim @GKotfis @ErikSchierboom @abecerramatias

public void A_robot_is_created_with_a_position_and_a_direction_robots_are_created_with_a_position_and_direction()
public void A_robot_is_created_with_a_position_and_a_direction_negative_positions_are_allowed()
public void Rotates_the_robots_direction_90_degrees_clockwise_changes_the_direction_from_north_to_east()
public void Rotates_the_robots_direction_90_degrees_clockwise_changes_the_direction_from_east_to_south()
public void Rotates_the_robots_direction_90_degrees_clockwise_changes_the_direction_from_south_to_west()
public void Rotates_the_robots_direction_90_degrees_clockwise_changes_the_direction_from_west_to_north()
public void Rotates_the_robots_direction_90_degrees_counter_clockwise_changes_the_direction_from_north_to_west()
public void Rotates_the_robots_direction_90_degrees_counter_clockwise_changes_the_direction_from_west_to_south()
public void Rotates_the_robots_direction_90_degrees_counter_clockwise_changes_the_direction_from_south_to_east()
public void Rotates_the_robots_direction_90_degrees_counter_clockwise_changes_the_direction_from_east_to_north()
public void Moves_the_robot_forward_1_space_in_the_direction_it_is_pointing_increases_the_y_coordinate_one_when_facing_north()
public void Moves_the_robot_forward_1_space_in_the_direction_it_is_pointing_decreases_the_y_coordinate_by_one_when_facing_south()
public void Moves_the_robot_forward_1_space_in_the_direction_it_is_pointing_increases_the_x_coordinate_by_one_when_facing_east()
public void Moves_the_robot_forward_1_space_in_the_direction_it_is_pointing_decreases_the_x_coordinate_by_one_when_facing_west()
public void Where_r_turn_right_l_turn_left_and_a_advance_the_robot_can_follow_a_series_of_instructions_and_end_up_with_the_correct_position_and_direction_instructions_to_move_east_and_north_from_readme()
public void Where_r_turn_right_l_turn_left_and_a_advance_the_robot_can_follow_a_series_of_instructions_and_end_up_with_the_correct_position_and_direction_instructions_to_move_west_and_north()
public void Where_r_turn_right_l_turn_left_and_a_advance_the_robot_can_follow_a_series_of_instructions_and_end_up_with_the_correct_position_and_direction_instructions_to_move_west_and_south()
public void Where_r_turn_right_l_turn_left_and_a_advance_the_robot_can_follow_a_series_of_instructions_and_end_up_with_the_correct_position_and_direction_instructions_to_move_east_and_north()

Copy link
Contributor

@SaschaMann SaschaMann Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide some concrete code examples of where my changes cause harm?

At least in the current form of the schema, it uses the description for something it's not intended to be, thus making the file less human-readable and more annoying to generate for generators that rely on the description being descriptive. I gave examples for both these points above.

So I don't understand what "other options" I can infer from them.

Ways to generate test suites without using the descriptions as method names directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the current form of the schema, it uses the description for something it's not intended to be, thus making the file less human-readable and more annoying to generate for generators that rely on the description being descriptive.

@SaschaMann Please note, I'd prefer not to change these descriptions at all. I'm only doing so as I was directed to here.

"cases": [
{
"description": "instructions to move east and north from README",
"description": "moving east and north",
"comments": [ "instructions to move east and north from README" ],
"property": "move",
"input": {
"position": {
Expand All @@ -316,7 +338,8 @@
}
},
{
"description": "instructions to move west and north",
"description": "moving west and north",
"comments": [ "instructions to move west and north" ],
"property": "move",
"input": {
"position": {
Expand All @@ -335,7 +358,8 @@
}
},
{
"description": "instructions to move west and south",
"description": "moving west and south",
"comments": [ "instructions to move west and south" ],
"property": "move",
"input": {
"position": {
Expand All @@ -354,7 +378,8 @@
}
},
{
"description": "instructions to move east and north",
"description": "moving east and north",
"comments": [ "instructions to move east and north" ],
"property": "move",
"input": {
"position": {
Expand Down