bugfix/prevent base number remapping #55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
============================================
+ Coverage 99.70% 99.85% +0.15%
- Complexity 354 357 +3
============================================
Files 54 54
Lines 1336 1341 +5
Branches 42 41 -1
============================================
+ Hits 1332 1339 +7
+ Partials 4 2 -2
Continue to review full report at Codecov.
|
| @@ -60,8 +59,7 @@ private String twoDigitsNumberAsString(Integer value) { | |||
| private String threeDigitsNumberAsString(Integer value) { | |||
| Integer tensWithUnits = value % 100; | |||
| Integer hundreds = value - tensWithUnits; | |||
| boolean hasNextNumber = tensWithUnits != 0; | |||
| return format("%s e %s", asWords(hundreds, hasNextNumber), asWords(tensWithUnits)); | |||
| return format("%s e %s", asWords(hundreds, true), asWords(tensWithUnits)); | |||
There was a problem hiding this comment.
hasNextNumber was always true, since all full hundreds (100, 200, 300...) are base numbers or an exception (100 is an exception in Portuguese), so they are handled in line 36 and 41
There was a problem hiding this comment.
hm, can we add a test for that one specific case?
There was a problem hiding this comment.
they are tested in BrazilianPortugueseValuesTest.groovy:59-67 – test cases for full hundred numbers (100 to 900)
|
|
||
| then: | ||
| thrown(NumberAlreadyMappedException) | ||
| } |
There was a problem hiding this comment.
You can also add test:
def 'i dont know which name is suitable for this test ;)' () {
given:
builder.put(1, new GenderForms("some form"))
when:
builder.put(1, "some other form")
then:
thrown(NumberAlreadyMappedException)
}
There was a problem hiding this comment.
good idea. Also added test for String -> GenderForm
|
|
||
| class BaseNumbersBuilderTest extends Specification { | ||
|
|
||
| def builder = baseNumbersBuilder() |
There was a problem hiding this comment.
I think it can be not good practice to use shared not-immutable object over multiple test cases. Its better to init new instance inside every test. In these 2 tests it doesnt change anything, but in future tests and developing new functionality - it can produce bugs.
There was a problem hiding this comment.
right. Moved initialization to setup method
No description provided.