Skip to content

meetup: improve descriptions by saying why each case is tested #1919

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

Merged
merged 4 commits into from
Feb 6, 2022

Conversation

petertseng
Copy link
Member

descriptions show whether a date is the first, last, or an arbitrary
middle date of the week. This helps understand why certain cases are
selected.

Closes #974

Please note the code that generated the descriptions as well.

@petertseng
Copy link
Member Author

petertseng commented Jan 14, 2022

search for the string "another" (11 10 occurrences) where I was unable to discern a specific reason for a given test case. If you can figure out the reason, the descriptions for those can be improved.

Although analysing the new descriptions may indicate gaps in coverage, adding cases to fill those gaps is not for this PR to do.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM

Note: some tracks have issues with fairly long descriptions. I don't know if the current descriptions would be considered long for those tracks, but I wanted to note it.

@@ -1131,7 +1131,7 @@
},
{
"uuid": "00c3ce9f-cf36-4b70-90d8-92b32be6830e",
"description": "first Friday of December 2012",
"description": "first Friday in a month where it falls on the 7th, the last day of the first week",
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know why this case is all the way down here. but I don't think moving it should be the job of this PR

@petertseng
Copy link
Member Author

petertseng commented Jan 14, 2022

The longest description is "second Thursday in a month where it falls on another arbitrary day in the middle of the second week" at 99 characters. There are ways of shortening it; we'd just have to decide what information we're willing to sacrifice in the description, and/or what words can be safely dropped while losing no information.

for example, if we changed "in a month where" to "when", we'd have "second Thursday when it falls on another arbitrary day in the middle of the second week" and it'd save 12 characters.

Another solution is to group test cases together, for example all of the second Tuesday cases would be grouped together, the "second Tuesday" prefix removed from each of their descriptions, and the test runner would show the group in an appropriately-labeled way, such as

second Tuesday
  when it falls on the 8th, the first day of the second week - PASS
  when it falls on the 14th, the last day of the second week - PASS
  when it falls on an arbitrary day in the middle of the second week - PASS

that's an option that can be explored if needed, assuming we're comfortable relying on everyone doing the right thing with these descriptions that only make sense when combined with their group description.

@ErikSchierboom
Copy link
Member

for example, if we changed "in a month where" to "when", we'd have "second Thursday when it falls on another arbitrary day in the middle of the second week" and it'd save 12 characters.

This is definitely an option. Would changing "when it falls on" to "is" also be an option?

Another solution is to group test cases together

It is, but only for tracks that support nested testS. I think the track that had issues with longer test names was the C track, which might or might not support nested tests. @wolf99 am I correct in remembering that the C or C++ track had issues with long test names?

@wolf99
Copy link
Contributor

wolf99 commented Jan 19, 2022

The C track does not "like" long test names. Technically they can be used but it results in quite ugly formatting, which can get confusing for humans.

For nested test cases, the C track usually concatenates the nesting name to avoid duplicate names across different nestings.

I am not sure about the C++ track.

@ErikSchierboom
Copy link
Member

The C track does not "like" long test names. Technically they can be used but it results in quite ugly formatting, which can get confusing for humans.

Do you know what the limit is for a long test name?

For nested test cases, the C track usually concatenates the nesting name to avoid duplicate names across different nestings.

Ah, this is also what I do for C# and F#, so that doesn't really help with the length of the tests.

@petertseng
Copy link
Member Author

Okay. So as a comparison:

second Thursday in a month where it falls on another arbitrary day in the middle of the second week
when second Thursday is another arbitrary day in the middle of the second week

that saves 21 characters.

@ErikSchierboom
Copy link
Member

Yeah, that sounds fairly sensible too me. Could "is another arbitrary" be shortened to "is an arbitrary"?

Is that still too much @wolf99?

@wolf99
Copy link
Contributor

wolf99 commented Jan 19, 2022

This seems a good compromise (we don't want to get too convoluted purely to achieve terseness) 😊

@petertseng
Copy link
Member Author

For cases with "another", I cannot change to "an" because there is already a case using "an"

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM

@ErikSchierboom
Copy link
Member

CC @exercism/reviewers

@BethanyG BethanyG self-requested a review January 26, 2022 12:06
Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

LGTM

@kotp
Copy link
Member

kotp commented Feb 6, 2022

"is another arbitrary"

Can be changed to "any" for either or both of them. "Arbitrary" and "any" are often interchangeable in meaning (just arbitrary makes one sound smarter?)

@petertseng
Copy link
Member Author

petertseng commented Feb 6, 2022

thinking about it, you're right.

so for the test cases:

when first Thursday is an arbitrary day in the middle of the first week
when first Thursday is another arbitrary day in the middle of the first week

I would then change those two to:

when first Thursday is some day in the middle of the first week
when first Thursday is another day in the middle of the first week

thank you for the suggestion! Your suggestion has been applied and it saves 8 characters in the first case and 10 characters in the second! Great improvement!

descriptions show whether a date is the first, last, or an arbitrary
middle date of the week. This helps understand why certain cases are
selected.

Closes exercism#974
This helps understand the logic used to write the new descriptions.
This reverts commit 848ca1f.

There is no need to keep this code permanently; it is a one time run and
once it is run it is done.
@junedev
Copy link
Member

junedev commented Feb 6, 2022

I think the text length are very reasonable now. @petertseng Feel free to merge.

@petertseng
Copy link
Member Author

petertseng commented Feb 6, 2022

I'd say it's a bit of a policy grey area whether I should be merging it at this time, because even though there are three approvals total, some changes came after some approvals.

In this case I'm using my judgment to say that overall the main changes have been looked by many, and the subsequent changes were small and all had at least two pairs of eyes on each (no one person has made them unilaterally and without review), and are not particularly likely to be contentious.

As you have seen before, with potentially contentious changes I would actually dismiss some prior approvals so that more approvals are needed.

However this is relying on a judgment call (what constitutes "potentially contentious"?), so if I turn out to be in error, let's discuss how to avoid future erroneous judgments. For this one, I take responsibility for any problems that result.

@petertseng petertseng merged commit 37f7831 into exercism:main Feb 6, 2022
@petertseng petertseng deleted the meetupa branch February 6, 2022 12:54
@ErikSchierboom
Copy link
Member

Thanks @petertseng!

ErikSchierboom added a commit to ErikSchierboom/problem-specifications that referenced this pull request Feb 11, 2022
* Format using prettier (exercism#1917)

Format using prettier

* updated description of anagrams exercise (exercism#1928)

* updated description of anagrams

* changed anagram description to be one-sentence-per-line

* updated description of anagrams to use sets

* Update Licence

Give a look at the discussion in BR exercism#1930

* rational-numbers: test to reduce abs value (exercism#1938)

* Change saddle point references to row, column (exercism#1948)

* word-search: Add test case

* Update exercises/word-search/canonical-data.json

Agreed.

Co-authored-by: Erik Schierboom <[email protected]>

* meetup: improve descriptions by saying why each case is tested (exercism#1919)

descriptions show whether a date is the first, last, or an arbitrary
middle date of the week. This helps understand why certain cases are
selected.

Closes exercism#974

* word-search: Add cases checking for concatenation and wrapping

The author of this commit thinks that concatenation is highly unlikely,
but the wrapping might be useful to check in languages that allow
negative indices.

* `flatten-array` Add additional test cases (exercism#1953)

* Add additional test cases to flatten-array

* Update exercises/flatten-array/canonical-data.json

Co-authored-by: Peter Tseng <[email protected]>

Co-authored-by: BethanyG <[email protected]>
Co-authored-by: Peter Tseng <[email protected]>

* Fix bowling game copy (exercism#1955)

Fixes exercism#1954

* Add action to format code (exercism#1941)

* build(deps): bump DavidAnson/markdownlint-cli2-action (exercism#1952)

Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 5.0.0 to 5.1.0.
- [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@b3c3b40...744f913)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Reduced rational nr. should be in standard form. (exercism#1958)

* Reduced rational should be in standard form.

The current instructors fail to mention that a reduced rational number should always be rendered in standard form (without any negative value at the denominator).

* remove superflous blank lines; fix wording

* scale-generator: use flat and sharp symbols (exercism#1942)

* Update configlet part in README (exercism#1949)

Co-authored-by: ee7 <[email protected]>

* phone number: only one problem per test input (exercism#1959)

* [Phone Number] Only one problem per test input

Because the area code is not allowed to start with 0 or 1, inputs designed to elicit other errors should not use area codes that start with either of those digits.

* Respect immutability

* Correct field name: s/comment/comments/

* Comments should contain a list.

* Allow prettier to improve comments

* book-store: reorder keys

* darts: reorder keys

* grade-school: reorder keys

* hamming: reorder keys

* high-scores: reorder keys

* largest-series-product: reorder keys

* list-ops: reorder keys

* luhn: reorder keys

* triangle: reorder keys

* scale-generator: reorder keys

* saddle-points: reorder keys

* diffie-hellman: reorder keys

* collatz-conjecture: reorder keys

* anagram: reorder keys

* accumulate: reorder keys

* Add CI script to check correct order of keys

Co-authored-by: Bart Massey <[email protected]>
Co-authored-by: y8l <[email protected]>
Co-authored-by: Ivan Ivanov <[email protected]>
Co-authored-by: Damian C. Rossney <[email protected]>
Co-authored-by: mariohuq <[email protected]>
Co-authored-by: mariohuq <[email protected]>
Co-authored-by: Peter Tseng <[email protected]>
Co-authored-by: Peter Tseng <[email protected]>
Co-authored-by: AH WEI <[email protected]>
Co-authored-by: BethanyG <[email protected]>
Co-authored-by: Cedd Burge <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Davide Alberto Molin <[email protected]>
Co-authored-by: wolf99 <[email protected]>
Co-authored-by: June <[email protected]>
Co-authored-by: ee7 <[email protected]>
Co-authored-by: Leah Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meetup: Test descriptions are unhelpful.
6 participants