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

Standard test suite for pascals-triangle #362

Merged
merged 1 commit into from
Sep 13, 2016

Conversation

IanWhitney
Copy link
Contributor

@IanWhitney IanWhitney commented Sep 5, 2016

In surveying the current implementations, I found 2 main approaches:

  • Test 1, 2, 3, 4, 5 and 20 without any boundary checking
    • Ruby is a good example
  • Test 4 & 6 rows, and test boundaries

Also, some languages exposed specific functions for rows and last_row while some relied on built-in functions for this behavior.

In this approach I'm trying to meld what I see as the best of these worlds.

Rows and Last Row functions

I think these functions should be implemented by the students, so I'm highlighting them as functions to test.

Fewer Tests

Once a student has gotten 4 rows to work the probably have the algorithm down. No real reason to test larger numbers that I can see.

Test Boundaries

An upper-limit test seemed unnecessary and very language-dependent. But catching Null and negative inputs seemed like useful things to test.

@petertseng
Copy link
Member

In commit message and PR description, link that should go to Kotlin goes to Ruby instead

@petertseng
Copy link
Member

petertseng commented Sep 5, 2016

A curiousity I found when I did this exercise in Java was that Java's tests have a isTriangle function. (Though it only tested arrays of arrays where the subarrays were all the right size! If adding isTriangle I would also test things like [[1], [1]]). Anyway, I assume it is deliberate to not add these, since the one-liner in https://github.com/exercism/x-common/blob/master/pascals-triangle.yml just says "computes Pascal's triangle up to a given number of rows" and not "verify a triangle is Pascal's triangle". Please confirm you agree with that.

@IanWhitney
Copy link
Contributor Author

I didn't notice it in Java, but I did spot it in..Perl, I think. Yeah, I think it's outside the scope of the problem.

@IanWhitney IanWhitney force-pushed the pascal_triangle_test_suite branch from 10f596c to a6e1411 Compare September 5, 2016 18:05
},
{
"description": "no rows",
"count": null,
Copy link
Member

Choose a reason for hiding this comment

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

in some languages it will not be possible to pass null in place of an integer, but I assume they will just skip that.

Speaking of "no rows", should the input of 0 be tested?

Copy link
Contributor

@rbasso rbasso Sep 8, 2016

Choose a reason for hiding this comment

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

I think that, to keep consistency, zero could be a valid input returning an empty list [], if we really want to test it. It seems the natural base case to me.

Number of lines must be natural, so I agree that negatives can be considered invalid input if the type system cannot enforce positive numbers. That said, I would not criticize a solution returning an empty list for negative inputs.

Should we test those cases ❓

@IanWhitney IanWhitney force-pushed the pascal_triangle_test_suite branch from a6e1411 to 58ee2a8 Compare September 11, 2016 15:03
IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Sep 11, 2016
Official tests have not been merged yet, but I'm mostly following them
here.

exercism/problem-specifications#362

I thought I could use `should_panic` to test a negative row count, but
no. It won't compile since I'm violating type safety. I've left that
test in, but plan to remove it before merge.

Example code is naive. Just enough to pass the tests. A real example
solution will come later.
IanWhitney pushed a commit to IanWhitney/x-common that referenced this pull request Sep 13, 2016
@rbasso
Copy link
Contributor

rbasso commented Sep 13, 2016

Seems great! 👍

In surveying the current implementations, I found 2 main approaches:

- Test 1, 2, 3, 4, 5 and 20 without any boundary checking
  - [Ruby](https://github.com/exercism/xruby/blob/e50b1cbeabc3a82c1ed7800da112b0984dbbf01c/exercises/pascals-triangle/pascals_triangle_test.rb)
    is a good example
- Test 4 & 6 rows, and test boundaries
  - [Kotlin](https://github.com/exercism/xkotlin/blob/02ecc5a70719f0b0e741992f33879f880f7c407f/exercises/pascals-triangle/src/test/kotlin/PascalsTriangleTest.kt)
    is a good example

Also, some languages exposed specific functions for `rows` and
`last_row` while some relied on built-in functions for this behavior.

In this approach I'm trying to meld what I see as the best of these
worlds.

Rows function
____

I think these functions should be implemented by the students, so I'm
highlighting them as functions to test

We decided that `last_row` functions were not necessary, since they
would mostly only alias existing language functions (`pop`, `last`,
etc.)

Fewer Tests
----

Once a student has gotten 4 rows to work the probably have the
algorithm down. No real reason to test larger numbers that I can see.

Test Boundaries
----

An upper-limit test seemed unnecessary and very language-dependent. But
catching Nil and negative inputs seemed like useful things to test.
@IanWhitney IanWhitney force-pushed the pascal_triangle_test_suite branch from ab8f8f6 to 094bfd7 Compare September 13, 2016 13:30
@IanWhitney IanWhitney merged commit 9a0cc1b into exercism:master Sep 13, 2016
IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Sep 17, 2016
Official tests have not been merged yet, but I'm mostly following them
here.

exercism/problem-specifications#362

Example code is naive. Just enough to pass the tests. A real example
solution will come later.
@IanWhitney IanWhitney deleted the pascal_triangle_test_suite branch January 14, 2017 19:10
@IanWhitney IanWhitney restored the pascal_triangle_test_suite branch January 14, 2017 19:10
@IanWhitney IanWhitney deleted the pascal_triangle_test_suite branch January 14, 2017 19:10
emcoding pushed a commit that referenced this pull request Nov 19, 2018
Suggest adding BookKeeping near end of the file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants