-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add unit tests for LoyaltyCard class #1923
Open
Aglag257
wants to merge
6
commits into
CatimaLoyalty:main
Choose a base branch
from
Aglag257:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ce5ec2c
Fixed formatting and describeContents()
Aglag257 84537ad
Revert formatting changes and fix describeContents() to return 0
Aglag257 f7d8f70
LoyaltyCardTest and necessary changes
Aglag257 f1f38d1
better handling of the Parcelable implementation
Aglag257 889e98d
Final correct LoyaltyCardTest implementation
Aglag257 71c17e2
removing deprecated lines
Aglag257 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,4 +203,4 @@ public LoyaltyCard[] newArray(int size) { | |
return new LoyaltyCard[size]; | ||
} | ||
}; | ||
} | ||
} |
94 changes: 94 additions & 0 deletions
94
app/src/test/java/protect/card_locker/LoyaltyCardTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package protect.card_locker; | ||
|
||
import android.os.Parcel; | ||
|
||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.robolectric.RobolectricTestRunner; | ||
|
||
import java.math.BigDecimal; | ||
import java.util.Currency; | ||
import java.util.Date; | ||
|
||
import com.google.zxing.BarcodeFormat; | ||
|
||
import static org.junit.Assert.*; | ||
|
||
@RunWith(RobolectricTestRunner.class) | ||
public class LoyaltyCardTest { | ||
|
||
@Test | ||
public void testParcelable() { | ||
Date validFrom = new Date(); | ||
Date expiry = new Date(); | ||
BigDecimal balance = new BigDecimal("100.00"); | ||
Currency currency = Currency.getInstance("USD"); | ||
LoyaltyCard card = new LoyaltyCard(1, "Store A", "Note A", validFrom, expiry, balance, currency, "12345", "67890", CatimaBarcode.fromBarcode(BarcodeFormat.QR_CODE), 0xFF0000, 1, System.currentTimeMillis(), 10, 0); | ||
|
||
Parcel parcel = Parcel.obtain(); | ||
card.writeToParcel(parcel, card.describeContents()); | ||
|
||
parcel.setDataPosition(0); | ||
|
||
LoyaltyCard createdFromParcel = LoyaltyCard.CREATOR.createFromParcel(parcel); | ||
|
||
assertEquals(card.id, createdFromParcel.id); | ||
assertEquals(card.store, createdFromParcel.store); | ||
assertEquals(card.note, createdFromParcel.note); | ||
assertEquals(card.validFrom, createdFromParcel.validFrom); | ||
assertEquals(card.expiry, createdFromParcel.expiry); | ||
assertEquals(card.balance, createdFromParcel.balance); | ||
assertEquals(card.balanceType, createdFromParcel.balanceType); | ||
assertEquals(card.cardId, createdFromParcel.cardId); | ||
assertEquals(card.barcodeId, createdFromParcel.barcodeId); | ||
assertEquals(card.barcodeType.name(), createdFromParcel.barcodeType.name()); | ||
assertEquals(card.headerColor, createdFromParcel.headerColor); | ||
assertEquals(card.starStatus, createdFromParcel.starStatus); | ||
assertEquals(card.lastUsed, createdFromParcel.lastUsed); | ||
assertEquals(card.zoomLevel, createdFromParcel.zoomLevel); | ||
assertEquals(card.archiveStatus, createdFromParcel.archiveStatus); | ||
|
||
parcel.recycle(); | ||
} | ||
|
||
@Test | ||
public void testIsDuplicate_sameObject() { | ||
Date now = new Date(); | ||
BigDecimal balance = new BigDecimal("50.00"); | ||
Currency currency = Currency.getInstance("EUR"); | ||
LoyaltyCard card1 = new LoyaltyCard(1, "Store B", "Note B", now, now, balance, currency, "22222", "33333", CatimaBarcode.fromBarcode(BarcodeFormat.PDF_417), 0x00FF00, 1, System.currentTimeMillis(), 5, 1); | ||
|
||
assertTrue(LoyaltyCard.isDuplicate(card1, card1)); | ||
} | ||
|
||
@Test | ||
public void testIsDuplicate_differentObjects() { | ||
Date now = new Date(); | ||
BigDecimal balance1 = new BigDecimal("50.00"); | ||
BigDecimal balance2 = new BigDecimal("75.00"); | ||
Currency currency = Currency.getInstance("EUR"); | ||
LoyaltyCard card1 = new LoyaltyCard(2, "Store C", "Note C", now, now, balance1, currency, "44444", "55555", CatimaBarcode.fromBarcode(BarcodeFormat.DATA_MATRIX), 0x0000FF, 0, System.currentTimeMillis(), 15, 1); | ||
LoyaltyCard card2 = new LoyaltyCard(2, "Store C", "Note C", now, now, balance2, currency, "44444", "55555", CatimaBarcode.fromBarcode(BarcodeFormat.DATA_MATRIX), 0x0000FF, 0, System.currentTimeMillis(), 15, 1); | ||
|
||
assertFalse(LoyaltyCard.isDuplicate(card1, card2)); | ||
} | ||
|
||
@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()); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a good change, yeah 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.