Robot simulator descriptions too long #1525

bencoman · 2019-06-05T13:39:13Z

[Edit] Some language tracks (e.g. Pharo) individually identify tests. To auto-create these identifiers during code generation, the only data currently available is the canonical data "description" field. Some tracks insert this field directly into their generated code as a comment, and the "descriptions" are suitably verbose for that purpose. However such verbosity generates some very long identifiers (e.g. >150 characters).

Issue #1473 proposed to adding a distinct "identifier" field. Back-pressure there directed me to first try compressing "descriptions" to suit "identifier" generation. This PR is a trial at doing that - minimizing description length without losing information. However even if no substantive information is lost, I such compression may compromise the readability of comments generated from the "descriptions", so I'm interested in feedback on that here and in #1473.

P.S. When originally creating this PR I misunderstood the canonical data "comments" field was for maintainers only and unrelated to generated comments for students. Original text of this post...

Convert long descriptions into comments per #1473

Convert long descriptions into comments per... per #1473

SleeplessByte

It seems that these changes are simply making the descriptions shorter for the sake of being shorter. I don't actually think there was anything wrong with any of these descriptions.

#1473 (comment)

I'm also not a fan of the context-requiring descriptions.

"description": "at negative position facing South",
"comments": [ "Negative positions allowed" ],

This will not yield the same test result description. This one could be renamed to "Allows negative positions". Similarly "Create" is not a good description. "Creation" could be okay, but I prefer "Creates a robot with position and direction"

I don't know why we want single word or 3 word descriptions. You lose a lot of information in the test suite this way.

SleeplessByte · 2019-06-05T13:58:59Z

exercises/robot-simulator/canonical-data.json

+      "description": "Series of instructions",
+      "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"
+      ]


Missing ,

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.

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.

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.

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.

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 ;)

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()

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.

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.

comma was added

SaschaMann · 2019-06-05T18:44:45Z

exercises/robot-simulator/canonical-data.json

@@ -31,7 +33,8 @@
          }
        },
        {
-          "description": "Negative positions are allowed",
+          "description": "Allows negative positions",
+          "comments": [ "Negative positions allowed" ],


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)

Yes, the comment is superfluous here.

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.

ErikSchierboom · 2019-06-06T06:46:37Z

It seems that these changes are simply making the descriptions shorter for the sake of being shorter. I don't actually think there was anything wrong with any of these descriptions.

I agree here. Yes the names are long, but I'm not in favor of shortening them and move the actual details to the comments to be honest. I'm not opposed to seeing if we can get away with shortening the names without losing any information, but to me these changes are bit much.

sshine

I suppose that if there is a middle way where the description contains the same quality but is shorter for the benefit of test generators that create identifiers from the description (even if this is only the Pharo track for now), but not so short that they become indecipherable, nobody stands to lose.

Here are some ideas.

As for the "comments": ..., most of the ones that were added seem superfluous and can be removed.

It would only make sense to add a comment if it contains anything of value for track maintainers. So, for example, a comment that adds "from the README" does not warrant the addition of a comment, I think. As a track maintainer, this doesn't help me do anything.

As for the way the Pharo track generates test case identifiers: It seems that it concatenates the outer description and the inner description. Are there examples of canonical data for some exercises where the inner descriptions themselves don't make any sense without this concatenation?

I suppose that any kind of consistency between the relationship between an outer and an inner description will make it easier to generate names based on them. Do you have any input here wrt. path of least resistance, @bencoman?

sshine · 2019-06-06T06:54:06Z

exercises/robot-simulator/canonical-data.json

@@ -10,10 +10,12 @@
  ],
  "cases": [
    {
-      "description": "A robot is created with a position and a direction",
+      "description": "Creation",


Suggested change

"description": "Creation",

"description": "Create robot with position and direction",

sshine · 2019-06-06T06:57:08Z

exercises/robot-simulator/canonical-data.json

@@ -31,7 +33,8 @@
          }
        },
        {
-          "description": "Negative positions are allowed",
+          "description": "Allows negative positions",
+          "comments": [ "Negative positions allowed" ],


Yes, the comment is superfluous here.

sshine · 2019-06-06T06:58:06Z

exercises/robot-simulator/canonical-data.json

@@ -51,10 +54,12 @@
      ]
    },
    {
-      "description": "rotates the robot's direction 90 degrees clockwise",
+      "description": "Rotate clockwise",


Suggested change

"description": "Rotate clockwise",

"description": "Rotate 90 degrees clockwise",

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

sshine · 2019-06-06T06:58:30Z

exercises/robot-simulator/canonical-data.json

      "cases": [
        {
-          "description": "changes the direction from north to east",
+          "description": "north to east",


Suggested change

"description": "north to east",

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

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

sshine · 2019-06-06T06:58:54Z

exercises/robot-simulator/canonical-data.json

@@ -73,7 +78,8 @@
          }
        },
        {
-          "description": "changes the direction from east to south",
+          "description": "east to south",


Suggested change

"description": "east to south",

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

sshine · 2019-06-06T07:02:51Z

exercises/robot-simulator/canonical-data.json

      "cases": [
        {
-          "description": "increases the y coordinate one when facing north",
+          "description": "facing north",


Suggested change

"description": "facing north",

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

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

sshine · 2019-06-06T07:03:15Z

exercises/robot-simulator/canonical-data.json

@@ -235,7 +250,8 @@
          }
        },
        {
-          "description": "decreases the y coordinate by one when facing south",
+          "description": "facing south",


Suggested change

"description": "facing south",

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

sshine · 2019-06-06T07:03:37Z

exercises/robot-simulator/canonical-data.json

@@ -254,7 +270,8 @@
          }
        },
        {
-          "description": "increases the x coordinate by one when facing east",
+          "description": "facing east",


Suggested change

"description": "facing east",

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

sshine · 2019-06-06T07:03:55Z

exercises/robot-simulator/canonical-data.json

@@ -273,7 +290,8 @@
          }
        },
        {
-          "description": "decreases the x coordinate by one when facing west",
+          "description": "facing west",


Suggested change

"description": "facing west",

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

sshine · 2019-06-06T07:05:16Z

exercises/robot-simulator/canonical-data.json

@@ -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",


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.

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

ErikSchierboom · 2019-06-06T07:23:22Z

Are there examples of canonical data for some exercises where the inner descriptions themselves don't make any sense without this concatenation?

Quite a bunch in the C# track: https://github.com/exercism/csharp/search?q=TestMethodNameWithPath&unscoped_q=TestMethodNameWithPath (look for testMethod.TestMethodName = testMethod.TestMethodNameWithPath;). So example are forth, triangle and list-ops. Note that in many cases, I have to use the full path as otherwise the test method names are not unique.

bencoman · 2019-06-06T14:31:13Z

Taking a step back for a moment. What we are coming against here is having a single field serving two purposes...

method description
method identifier

then ending up serving neither purpose well.

The alternative is to have a separate field for each purpose per. Support #1473 and the need for this and subsequent PRs vanishes. I am somewhat dreading getting all of us bogged down in similar discussion for other exercises. But will have to keep pushing. Some of the generated names are truly awful.

bencoman · 2019-06-06T14:36:18Z

I see Scala concatenates nested descriptions into a comment. I think that works well as a comment, not so well as an identifier. The code panes of IDEs are typically much wider than the navigation panes.

And the way PHP nests descriptions as comments looks fine, but individual tests are annoymous, whereas other languages finely distinguish each test condition by name (at least Pharo and CSharp).

SleeplessByte · 2019-06-06T15:19:18Z

I am somewhat dreading getting all of us bogged down in similar discussion for other exercises. But will have to keep pushing. Some of the generated names are truly awful.

@bencoman I think your concerns are valid, raising fruitful discussion and we can clean-up a lot of these, but we just got to consider what it means for all the generators that already exist -- and that it can be a series of changes both in how your generator works and how ours work 👍

Sascha asked me to.

bencoman · 2019-06-10T17:59:48Z

@SleeplessByte thanks for your support. Just to clarify... my concern is nothing about these descriptions "as descriptions". I wouldn't want to change them except that they make poor identifiers.
Indeed now its been pointed out that the "comments" field is for maintainers not students, I'd like to generate these descriptions into code comments just as they are - but that would leave me with awful identifiers. I'm not really happy to be compromising the quality of the descriptions. I'd much prefer an optional "identifier" field

bencoman · 2019-06-10T19:09:28Z

middle way where the description contains the same quality but shorter for the benefit of test generators that create identifiers from the description, but not so short that they become indecipherable, nobody stands to lose.

@sshine Its the highly subjective nature of "same quality but shorter" that I'm concerned will burn everyone's time bike-shedding over. I don't really want to be corrupting descriptions to produce better identifiers. They serve different purposes and jamming both into one field compromises both. I'm coming more to believe its better to separate the concern for an "identifier" to a separate optional field.

SleeplessByte · 2019-06-10T20:36:33Z

@bencoman I got you. I understand what you're trying to do. Would you want adding that comment in #1496 ?

rpottsoh

Don't forget to properly bump the version at some point if this is to be merged.

ErikSchierboom

Great work!

ErikSchierboom · 2019-06-12T05:39:02Z

@SaschaMann You are listed as a pending reviewer. Could you perhaps take a look?

SleeplessByte

This is perfect @bencoman. Thanks for keeping with us.

@ErikSchierboom check slack dm.

ErikSchierboom · 2019-06-12T13:35:41Z

Thanks @bencoman! It really is appreciated.

ErikSchierboom · 2019-06-12T16:25:04Z

I've just submitted a PR for the C# track and the new descriptions are a huge improvement: exercism/csharp#1287

Comparing the current tests (3.1.0.7) with the current canonical version for this exercise (3.2.0), the descriptions and titles have changed in the PR exercism/problem-specifications#1525

Robot simulator descriptions too long

ea538f6

Convert long descriptions into comments per... per #1473

bencoman mentioned this pull request Jun 5, 2019

Query - reconciling existing tests with problem-specifications exercism/pharo-smalltalk#377

Open

SleeplessByte previously requested changes Jun 5, 2019

View reviewed changes

bencoman added 2 commits June 5, 2019 23:02

fix missing comma, thx @SleeplessByte

12a2ccc

update per suggestions from SleeplessByte

99706b8

SaschaMann previously requested changes Jun 5, 2019

View reviewed changes

sshine reviewed Jun 6, 2019

View reviewed changes

bencoman mentioned this pull request Jun 6, 2019

Proposal: Add canonical data "identifier" field to separate concern from test "description" #1473

Closed

bencoman mentioned this pull request Jun 6, 2019

ETL descriptions too long #1526

Merged

iHiD requested a review from SaschaMann June 7, 2019 23:46

petertseng mentioned this pull request Jun 10, 2019

list of tracks with test generators #1411

Closed

rpottsoh requested changes Jun 11, 2019

View reviewed changes

SleeplessByte mentioned this pull request Jun 11, 2019

Meta: adding new keys to the schema #1496

Open

make descriptions a bit more verbose

8cf9021

ErikSchierboom approved these changes Jun 11, 2019

View reviewed changes

rpottsoh approved these changes Jun 11, 2019

View reviewed changes

SleeplessByte approved these changes Jun 12, 2019

View reviewed changes

SleeplessByte merged commit 2d58f8e into master Jun 12, 2019

SleeplessByte deleted the bencoman-shorter-descriptions-robot-simulator branch June 12, 2019 13:35

thalesmg mentioned this pull request Oct 15, 2019

robot-simulator: bump version to 3.2.0 and update test titles exercism/haskell#872

Merged

	"description": "Creation",
	"description": "Create robot with position and direction",

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

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

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

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

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

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

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

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

Robot simulator descriptions too long #1525

Robot simulator descriptions too long #1525

Conversation

bencoman commented Jun 5, 2019 • edited Loading

SleeplessByte left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencoman Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

bencoman Jun 5, 2019 • edited by SleeplessByte Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaMann Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaMann Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaMann Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented Jun 6, 2019

sshine left a 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

bencoman Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencoman Jun 6, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented Jun 6, 2019

bencoman commented Jun 6, 2019

bencoman commented Jun 6, 2019 • edited Loading

SleeplessByte commented Jun 6, 2019

bencoman commented Jun 10, 2019

bencoman commented Jun 10, 2019

SleeplessByte commented Jun 10, 2019

rpottsoh left a comment

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Jun 12, 2019

SleeplessByte left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Jun 12, 2019

ErikSchierboom commented Jun 12, 2019

bencoman commented Jun 5, 2019 •

edited

Loading

SleeplessByte left a comment •

edited

Loading

bencoman Jun 5, 2019 •

edited

Loading

bencoman Jun 5, 2019 •

edited by SleeplessByte

Loading

SaschaMann Jun 5, 2019 •

edited

Loading

SaschaMann Jun 6, 2019 •

edited

Loading

SaschaMann Jun 5, 2019 •

edited

Loading

bencoman Jun 6, 2019 •

edited

Loading

bencoman Jun 6, 2019 •

edited

Loading

bencoman commented Jun 6, 2019 •

edited

Loading