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.

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

Add test generator for "binary" exercise #394

Merged
merged 4 commits into from
Jul 25, 2016
Merged

Conversation

tommyschaefer
Copy link
Contributor

@tommyschaefer tommyschaefer commented Jul 14, 2016

I recently came across #381, and saw that @kytrinyx suggesting converting all existing exercises to use generated tests as a first step.

I was wondering if this is still a goal? If so, I would love to help work on that! 😄

To start this off, I've added a generator for the "binary" example.

Thank you so much for your time, and I look forward to your feedback!

@tommyschaefer
Copy link
Contributor Author

tommyschaefer commented Jul 14, 2016

Also, here's the test data for the binary example:

x-common/binary.json

{
   "cases": [
      {
         "description": "binary 0 is decimal 0",
         "binary": "0",
         "expected": 0
      },
      {
         "description": "binary 1 is decimal 1",
         "binary": "1",
         "expected": 1
      },
      {
         "description": "binary 10 is decimal 2",
         "binary": "10",
         "expected": 2
      },
      {
         "description": "binary 11 is decimal 3",
         "binary": "11",
         "expected": 3
      },
      {
         "description": "binary 100 is decimal 4",
         "binary": "100",
         "expected": 4
      },
      {
         "description": "binary 1001 is decimal 9",
         "binary": "1001",
         "expected": 9
      },
      {
         "description": "binary 11010 is decimal 26",
         "binary": "11010",
         "expected": 26
      },
      {
         "description": "binary 10001101000 is decimal 1128",
         "binary": "10001101000",
         "expected": 1128
      },
      {
         "description": "binary ignores leading zeros",
         "binary": "000011111",
         "expected": 31
      },
      {
         "description": "invalid binary numbers raise an error",
         "binary": ["012", "10nope", "nope10", "10nope10", "001 nope", "2"],
         "expected": -1
      }
   ]
}

@tommyschaefer tommyschaefer force-pushed the master branch 4 times, most recently from f31af19 to f15f256 Compare July 15, 2016 03:45
@Insti
Copy link
Contributor

Insti commented Jul 15, 2016

Nice work @tommyschaefer, Yes this is something that still needs to be done.

Is this best for one pull request? Or should each addition be it's own PR?

I think each problem should be in it's own PR. There are often issues discovered during this process that require further discussion and these are best handled on a case-by-case basis and without needing to block other simpler ones.

Also, in some cases test data needs to be added to the exercism/x-common, so should I also open PR's on that repo?

Yes additions/modifications to the .json files will need to be added as PRs on the x-common repository.

@tommyschaefer tommyschaefer changed the title Add test generator for all exercises Add test generator for "binary" exercise Jul 15, 2016
@tommyschaefer
Copy link
Contributor Author

tommyschaefer commented Jul 15, 2016

Okay great! Thank you so much for the feedback, @Insti!

I've updated the description and title of this pull request to only refer to the binary exercise. Also, I opened a PR at exercism/problem-specifications#296 to add the JSON test data above!

@Insti
Copy link
Contributor

Insti commented Jul 15, 2016

@tommyschaefer will you create an issue here in the xruby repository with your list of problems that require generators so we can track progress towards implementing them all?

@Insti
Copy link
Contributor

Insti commented Jul 15, 2016

You should probably also be mindful of #386 - "Audit test files for assert/refute with no custom message"
I think the general intention is that assertion failures have helpful error messages.

@tommyschaefer
Copy link
Contributor Author

Will you create an issue here in the xruby repository with your list of problems that require generators so we can track progress towards implementing them all?

Absolutely! This is now at: #396

You should probably also be mindful of #386 - "Audit test files for assert/refute with no custom message"
I think the general intention is that assertion failures have helpful error messages.

Ah! Excellent! Thank you for pointing that out 😄 I didn't see that initially, so I will keep that in mind and make some adjustments!

@Insti
Copy link
Contributor

Insti commented Jul 15, 2016

Before you spend too much time working on the generator for binary you probably should know that it is being depricated. So it might be better to work on one of the other problems.

@kotp
Copy link
Member

kotp commented Jul 15, 2016

Good catch on that @Insti though it is a good practice one. It is simple, and doesn't have nested cases.

@tommyschaefer
Copy link
Contributor Author

Ah yes! I didn't see that before. Thank's for the heads up @Insti 😄

@Insti
Copy link
Contributor

Insti commented Jul 22, 2016

Merge this? 👍 👎 ❓

Argument for: The work has been done, it looks good, the problem is not deprecated yet.
Minor issue: Not sure the test file changes warrant a version bump from 2 to 3.

@kotp
Copy link
Member

kotp commented Jul 22, 2016

Version bump can happen, we will make more room for it.

@kytrinyx
Copy link
Member

If solutions that pass the ungenerated test file would also pass the generated one, I wouldn't change the version.

@Insti
Copy link
Contributor

Insti commented Jul 23, 2016

@tommyschaefer are you able to change the test version back from 3 to 2?
Then we can merge this.

@tommyschaefer
Copy link
Contributor Author

@Insti Absolutely! I've changed the test version back to 2 😄

@Insti Insti merged commit f319c72 into exercism:master Jul 25, 2016
@Insti
Copy link
Contributor

Insti commented Jul 25, 2016

Thanks for your work on this @tommyschaefer

@Insti
Copy link
Contributor

Insti commented Jul 25, 2016

I think I made a mistake by merging this before the x-common data was complete.
I've decided to "fail forwards" by merging that in as well rather than trying to back this change out and creating more confusion than necessary.

gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016
Existing tests:
* https://github.com/exercism/xelixir/blob/master/exercises/forth/forth_test.exs
* https://github.com/exercism/xfsharp/blob/master/exercises/forth/ForthTest.fs
* https://github.com/exercism/xhaskell/blob/master/exercises/forth/test/Tests.hs
* https://github.com/exercism/xrust/blob/master/exercises/forth/tests/forth.rs
* https://github.com/exercism/xscala/blob/master/exercises/forth/src/test/scala/ForthTest.scala

All tracks had the exact same tests, with two exceptions:

First, The "All non-word characters are separators test". In particular,
F# chose not to include the Ogham space mark, and I argue the same in
exercism/haskell#388.

The Ogham space mark adds no additional value (it is just another space
character) and is confusing (it is visually too similar to the minus
sign), so it will not be included.

Second, Rust has a few extras in its tests:

* Parsing negative numbers (adds an extra wrinkle since now we must
  determine whether a minus is unary or binary)
* Extra error cases: +, -, *, / with not enough arguments.
* Ensuring that user-defined words are case-insensitive: defines `CoUnT`
  then executes `count COUNT`
* Ensuring that user-defined words execute their definitions in the
  right order.
* Extra error cases: Incomplete definitions (a `:` without a `;`)

After discussion in exercism#394, we decide that we will only add tests for the
second and fourth of these.

Further, we have changed the expectations from the string representation
of the stack to simply an array/list of values on the stack.

Closes https://github.com/exercism/todo/issues/92
gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016
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.

4 participants