Add unit tests for LoyaltyCard class #1923
Conversation
| import static org.junit.Assert.*; | ||
|
|
||
| @RunWith(RobolectricTestRunner.class) | ||
| @Config(manifest=Config.NONE) |
There was a problem hiding this comment.
Just curious, why this line? No other tests seem to use it.
There was a problem hiding this comment.
Wondering the same. It also seems to be deprecated.
There was a problem hiding this comment.
Thank you for bringing this to my attention.
The @config(manifest=Config.NONE) line was included to ensure that Robolectric tests run without relying on an Android manifest file. However, I see that the manifest field within the @config annotation is marked as deprecated. This means that we should migrate to the preferred way of configuring builds as recommended in the Robolectric documentation.
I will update the tests to remove the deprecated usage and follow the new guidelines
|
|
||
| @Test | ||
| public void testToString() { | ||
| Date now = new Date(); | ||
| BigDecimal balance = new BigDecimal("100.00"); | ||
| Currency currency = Currency.getInstance("USD"); | ||
| LoyaltyCard card = new LoyaltyCard(3, "Store D", "Note D", now, now, balance, currency, "66666", "77777", CatimaBarcode.fromBarcode(BarcodeFormat.AZTEC), null, 2, System.currentTimeMillis(), 20, 2); | ||
|
|
||
| String expected = String.format( | ||
| "LoyaltyCard{%n id=%s,%n store=%s,%n note=%s,%n validFrom=%s,%n expiry=%s,%n" | ||
| + " balance=%s,%n balanceType=%s,%n cardId=%s,%n barcodeId=%s,%n barcodeType=%s,%n" | ||
| + " headerColor=%s,%n starStatus=%s,%n lastUsed=%s,%n zoomLevel=%s,%n archiveStatus=%s%n}", | ||
| card.id, card.store, card.note, card.validFrom, card.expiry, | ||
| card.balance, card.balanceType, card.cardId, card.barcodeId, | ||
| card.barcodeType != null ? card.barcodeType.format() : null, | ||
| card.headerColor, card.starStatus, card.lastUsed, card.zoomLevel, card.archiveStatus); | ||
|
|
||
| assertEquals(expected, card.toString()); | ||
| } |
There was a problem hiding this comment.
I don't think this test is really useful, I don't think the toString is ever used except to be able to use in quick debugging.
Although I guess not testing this would make test coverage tooling say this code isn't tested (even though I don't think it's important to test).
Eh, no strong opinion I guess. Do you have an opinion @obfusk?
There was a problem hiding this comment.
I have to agree it doesn't really serve much purpose, especially as it currently duplicates the exact same String.format() from the .toString() code.
If you really want a regression test for code like this I would recommend comparing against a hardcoded string of the expected output so you can at least confirm the output looks like it's supposed to and aren't essentially just checking whether two identical calls to String.format() produce the same result.
I'd say more test coverage is generally good as long as it doesn't require effort to write and maintain the tests that could better be spent on more important things :)
There was a problem hiding this comment.
That would be a good change, yeah 👍
There was a problem hiding this comment.
Thank you both for your feedback and suggestions.
I understand your point about the current implementation of the testToString method. I initially included it to have a more complete test class but It indeed duplicates the String.format logic, which may not add significant value to the test coverage. To address this, I can update the test to compare the toString output against a hardcoded expected string. This will help ensure that the output is as expected without duplicating the code logic.
|
Given that |
You're right; given that isDuplicate() is designed to ignore differences in lastUsed and zoomLevel, it makes sense to include tests that verify this behavior. I will add additional tests to ensure that isDuplicate correctly identifies duplicate LoyaltyCard instances even when lastUsed and zoomLevel values differ. |
This improves test coverage and ensures the correct behavior of the LoyaltyCard class.