tad-lispy commented 5 years ago

Some functions and decoders are covered by doc-tests (examples with expected return values). The tests are performed as part of Make targets: verify-examples and tests.

Some more complex cases (like viewSalary with all data provided) were intentionally ommited. The examples are primarily for human readers, so they shouldn't be too complex. If we need to test these complex scenarios, we should use unit or integration tests.

Notes

This PR supersedes #22

Unfortunately the doc tests currently do not get counted in coverage report. This is due to how Elm Validate Examples work. To change this first that issue would have to be addressed: https://github.com/stoeffel/elm-verify-examples/issues/65

Another improvement would be to follow the advise here https://github.com/elm-explorations/test/tree/1.2.1#application-specific-techniques and not expose all the internal helpers from Main module. We could create several specialized modules like Salary, City, Tenure or just one Internal with everything exposed and one Main which would only expose main. IMO this goes beyond the scope of this PR. Maybe we should make an issue for that?

codecov[bot] commented 5 years ago

Codecov Report

Merging #29 into master will increase coverage by 1.35%. The diff coverage is 59.09%.

@@             Coverage Diff              @@
##             master      #29      +/-   ##
============================================
+ Coverage     40.51%   41.86%   +1.35%     
- Complexity     1761     1904     +143     
============================================
  Files             1        1              
  Lines           501      535      +34     
============================================
+ Hits            203      224      +21     
- Misses          298      311      +13

Impacted Files	Coverage Δ	Complexity Δ
src/SalaryCalculator.elm	`41.86% <59.09%> (+1.35%)`	`1904 <170> (+143)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b171838...1be5769. Read the comment docs.

zupo commented 5 years ago

Another improvement would be to follow the advise here https://github.com/elm-explorations/test/tree/1.2.1#application-specific-techniques and not expose all the internal helpers from Main module. We could create several specialized modules like Salary, City, Tenure or just one Internal with everything exposed and one Main which would only expose main. IMO this goes beyond the scope of this PR. Maybe we should make an issue for that?

Yes.

zupo commented 5 years ago

Unfortunately the doc tests currently do not get counted in coverage report. This is due to how Elm Validate Examples work. To change this first that issue would have to be addressed: stoeffel/elm-verify-examples#65

Makes sense. Please work on getting https://github.com/stoeffel/elm-verify-examples/issues/65 resolved, merged & release.

tad-lispy commented 5 years ago

Please work on getting stoeffel/elm-verify-examples#65 resolved, merged & release.

My PR finally got merged and released! https://github.com/stoeffel/elm-verify-examples/pull/84

teamniteo / salarycalc