Skip to content

Remove unused function testCase generating warning when compiling test modules.#146

Merged
kytrinyx merged 1 commit intoexercism:masterfrom
rbasso:unused-testcase
Jun 14, 2016
Merged

Remove unused function testCase generating warning when compiling test modules.#146
kytrinyx merged 1 commit intoexercism:masterfrom
rbasso:unused-testcase

Conversation

@rbasso
Copy link
Copy Markdown
Contributor

@rbasso rbasso commented Jun 8, 2016

Some exercises show a warning when compiling the test module:

Defined but not used: `testCase`

This can confuse users and is not considered good practice.

Unused code:

  • function testCase
  • import of Assertion

Affected exercises:

  • atbash-cipher
  • binary
  • crypto-square
  • hexadecimal
  • largest-series-product
  • nth-prime
  • prime-factors
  • raindrops
  • roman-numerals
  • scrabble-score
  • triangle

Closes #145

Removed code:

- function testCase
- import of Assertion

Affected exercises:
    atbash-cipher
    binary
    crypto-square
    hexadecimal
    largest-series-product
    nth-prime
    prime-factors
    raindrops
    roman-numerals
    scrabble-score
    triangle
@kytrinyx
Copy link
Copy Markdown
Member

I can't think of a reason to keep the code that (a) we are not using and (b) causes a warning.

@towynlin Is there something here that I'm missing?

@petertseng
Copy link
Copy Markdown
Member

Now you are getting me curious as to why Travis doesn't fail without this PR, as we do run tests with -Wall -Werror...

Experimenting quickly on my machine, some raw results:

  • neither runghc -Wall -Werror not runhaskell -Wall -Werror emit errors
  • ghc -Wall -Werror fails as you would expect: binary_test.hs:11:1: Warning: Defined but not used: ‘testCase
  • there are cases in which runhaskell will emit errors: if I have an unused binding in Binary.hs, only then we get something: Binary.hs:7:1: Warning: Defined but not used: ‘a’ - perhaps runhaskell does something to ignore unused errors from the main module... or something? I would benefit greatly from an explanation here!

As for this PR:

The pattern we will see is that we'll have two lines mentioning testCase in the file.

$ git grep testCase | cut -d: -f1 | uniq -c | sort -n
      2 atbash-cipher/atbash-cipher_test.hs
      2 binary/binary_test.hs
      2 crypto-square/crypto-square_test.hs
      2 hexadecimal/hexadecimal_test.hs
      2 largest-series-product/largest-series-product_test.hs
      2 nth-prime/nth-prime_test.hs
      2 prime-factors/prime-factors_test.hs
      2 raindrops/raindrops_test.hs
      2 roman-numerals/roman-numerals_test.hs
      2 scrabble-score/scrabble-score_test.hs
      2 triangle/triangle_test.hs
      3 sgf-parsing/sgf-parsing_test.hs
...

(further entries omitted, we are only interested in entries with 2 lines, that means we also don't touch sgf-parsing)

this seems to match the list in this PR, so this all seems good - it is certain that testCase is unused in the files mentioned in this PR (and if it were being used, Travis would have failed).

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Jun 11, 2016

I imagine the reason why testCase was present was... copypasta, probably.

The only test files that don't have testCase in them...

$ git grep -L testCase */*_test.hs
food-chain/food-chain_test.hs
house/house_test.hs
octal/octal_test.hs
trinary/trinary_test.hs

So probably some copy paste that includes testCase by default, even though it's not used everywhere (since it was used in a majority).

@rbasso
Copy link
Copy Markdown
Contributor Author

rbasso commented Jun 11, 2016

Same result here regarding runhaskell. It shows some warnings but not others, but I don't have an explanation for that.

I got the warnings because I have a very different setup here. I don't like the use of a global installation of GHC with the packages provided by _test/bootstrap.sh, so I'm using stack projects for all exercises in my repository of solutions - even in Travis CI - to have better isolation, replicability and track dependencies more precisely.

I really prefer stack test over _test/check-exercises.

@petertseng
Copy link
Copy Markdown
Member

To me personally, it seems valuable to track dependencies per exercise instead of globally. I definitely took that approach for a different project with multiple self-contained problems. I wonder if one day that would be an appropriate thing to do for this repo.

I realize what I said is not really relevant to this PR so maybe suited for a different issue.

@towynlin
Copy link
Copy Markdown
Contributor

Thanks for pinging me @kytrinyx — haven't found the time to review yet. Good discussion here. Keep it up folks. I'm not hearing a conclusive merge/close call from the discussion yet. If anybody is confident one way or the other, please say so!

@petertseng
Copy link
Copy Markdown
Member

For what it's worth, I would support merging this.

@rbasso
Copy link
Copy Markdown
Contributor Author

rbasso commented Jun 14, 2016

We where just discussing others issues in the wrong place. Sorry!
About this PR I believe everyone here agrees.

@kytrinyx
Copy link
Copy Markdown
Member

Cool, in that case... :merge:

@kytrinyx kytrinyx merged commit 8eabd65 into exercism:master Jun 14, 2016
@towynlin
Copy link
Copy Markdown
Contributor

I see — finally got a minute to glance over the code. Appreciate your git | cut | uniq | sort data mining @petertseng — that makes very clear that this is the right change. Thanks @rbasso!

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