Test: Local Snapshot unit tests #1364

kwek20 · 2019-02-27T18:43:27Z

Description

Added unit tests to all other local snapshot files
Added javadoc to LS related config

Fixes #1091

Cannot be merged before merging of #1363

Type of change

Enhancement (a non-breaking change which adds functionality)
Documentation Fix

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

GalRogozinski · 2019-04-29T09:19:43Z

src/main/java/com/iota/iri/conf/ProtocolConfig.java

    double getpSendMilestone();

+    /**
+     * @return Descriptions#P_PROPAGATE_REQUEST
+     */


It is missing @value annotation.
You made this mistake because you copy pasted code that was never reviewed (I guess this is why reviews are important).

Anyhows #1159 should fix this so you can simply discard the changes to this file

GalRogozinski · 2019-04-29T09:23:19Z

src/main/java/com/iota/iri/service/snapshot/impl/LocalSnapshotManagerImpl.java

@@ -128,7 +129,8 @@ public void shutdown() {
     *
     * @param latestMilestoneTracker tracker for the milestones to determine when a new local snapshot is due
     */
-    private void monitorThread(LatestMilestoneTracker latestMilestoneTracker) {
+    //Package Private For Testing


You can use the @VisibleForTesting no?

GalRogozinski · 2019-04-29T09:23:43Z

src/main/java/com/iota/iri/service/snapshot/impl/LocalSnapshotManagerImpl.java

@@ -154,38 +156,42 @@ private void monitorThread(LatestMilestoneTracker latestMilestoneTracker) {
     * @param inSync if this node is in sync
     * @return the current interval in which we take local snapshots
     */
-    private int getSnapshotInterval(boolean inSync) {
+    //Package Private For Testing


GalRogozinski · 2019-04-29T09:23:54Z

src/main/java/com/iota/iri/service/snapshot/impl/LocalSnapshotManagerImpl.java

     * 
     * @param latestMilestoneTracker tracker we use to determine milestones
     * @return <code>true</code> if we are in sync, otherwise <code>false</code>
     */
-    private boolean isInSync(LatestMilestoneTracker latestMilestoneTracker) {
+    // Package Private For Testing


GalRogozinski · 2019-04-29T09:32:47Z

src/test/java/com/iota/iri/service/snapshot/impl/LocalSnapshotManagerImplTest.java

+                lsManager.monitorThread(milestoneTracker);
+
+            }
+        });


Why not use lambda?
new Thread( () -> lsManager.monitorThread(milestoneTracker))

GalRogozinski · 2019-04-29T10:49:42Z

src/test/java/com/iota/iri/service/snapshot/impl/SnapshotProviderImplTest.java

+    }
+
+    @After
+    public void tearDown() throws Exception {


throws Exception should be deleted...
Doesn't your IDE tell you this?

It did not! :(

So why didn't you delete?

GalRogozinski · 2019-04-29T10:56:07Z

src/test/java/com/iota/iri/service/snapshot/impl/SnapshotStateImplTest.java

+    @Test
+    public void testGetBalances() {
+        assertEquals("State should not have balances", new HashMap<>(), state.getBalances());
+        assertEquals("State should have the ballances it was created with", map, balanceState.getBalances());


typo "ballances"

GalRogozinski · 2019-04-29T10:57:06Z

src/test/java/com/iota/iri/service/snapshot/impl/SnapshotStateImplTest.java

+        state.applyStateDiff(diff);
+
+        long balance = state.getBalance(Hash.NULL_HASH);
+        assertEquals("Applying state to an empty state should have 5 for genisis", 5l, balance);


GalRogozinski · 2019-04-29T11:01:58Z

src/test/java/com/iota/iri/service/snapshot/impl/SnapshotStateImplTest.java

+        patchedState = balanceState.patchedState(diff);
+
+        long balance = patchedState.getBalance(Hash.NULL_HASH);
+        assertEquals("5 should have been added to genisis", TransactionViewModel.SUPPLY - 5l, balance);


GalRogozinski · 2019-04-29T11:06:01Z

src/main/java/com/iota/iri/service/snapshot/impl/SnapshotProviderImpl.java

@@ -62,7 +62,8 @@
     *       snapshot multiple times while creating their own version of the LocalSnapshotManager, we cache the instance
     *       here so they don't have to rebuild it from the scratch every time (massively speeds up the unit tests).
     */
-    private static SnapshotImpl builtinSnapshot = null;
+    // Package private For Testing
+    static SnapshotImpl builtinSnapshot = null;


It would be better to have @VisibleForTesting method that nullfies the bulitinSnapshot

Let's not expose more than we need to

You know something. This shouldn't be a static field. Can you open an issue for that please?

…out testnet config test

GalRogozinski

Looks pretty good.
Just delete the throws, add a comment, and open an issue and I shall approve

GalRogozinski · 2019-05-08T10:54:59Z

src/test/java/com/iota/iri/service/snapshot/impl/SnapshotProviderImplTest.java

+    }
+
+    @After
+    public void tearDown() throws Exception {


So why didn't you delete?

GalRogozinski · 2019-05-08T10:56:53Z

src/test/java/com/iota/iri/service/snapshot/impl/LocalSnapshotManagerImplTest.java

+        this.lsManager = new LocalSnapshotManagerImpl();
+
+        lsManager.init(snapshotProvider, snapshotService, transactionPruner, config);
+        when(snapshotProvider.getLatestSnapshot().getIndex()).thenReturn(-5, -1, 10, 998, 999, 1999, 2000);


Hmm I don't know....
I would add a comment in the test code that we don't really support negative indexes

GalRogozinski · 2019-05-08T11:02:33Z

src/main/java/com/iota/iri/service/snapshot/impl/SnapshotProviderImpl.java

@@ -62,7 +62,8 @@
     *       snapshot multiple times while creating their own version of the LocalSnapshotManager, we cache the instance
     *       here so they don't have to rebuild it from the scratch every time (massively speeds up the unit tests).
     */
-    private static SnapshotImpl builtinSnapshot = null;
+    // Package private For Testing
+    static SnapshotImpl builtinSnapshot = null;


You know something. This shouldn't be a static field. Can you open an issue for that please?

kwek20 changed the title ~~Ls extra unit tests~~ Test: Local Snapshot unit tests Feb 27, 2019

kwek20 mentioned this pull request Feb 27, 2019

Test: Additional unit tests to Transaction, DAGHelper and WereAddressesSpentFromProvider #1365

Merged

6 tasks

iotaledger deleted a comment Feb 28, 2019

kwek20 force-pushed the ls-extra-unit-tests branch from a2fe772 to f054f90 Compare March 15, 2019 00:24

kwek20 force-pushed the ls-extra-unit-tests branch from f054f90 to 19a2465 Compare April 24, 2019 16:28

GalRogozinski suggested changes Apr 29, 2019

View reviewed changes

kwek20 and others added 7 commits April 29, 2019 19:31

Added LS tests, modified javadoc

90736c6

Fixed 2 broken tests, worked around cached buildin snapshot breaking …

56e0ef7

…out testnet config test

Added codacy fixes

0ba53dd

Updated method names

998361b

Removed javadoc on favor of PR iotaledger#1159

f923d32

Changed comments to @VisibleForTesting

63fe6f0

Typos fixed

203dd16

kwek20 force-pushed the ls-extra-unit-tests branch from 19a2465 to 203dd16 Compare April 29, 2019 17:57

Added import statement, how did that not error?

f5119d4

GalRogozinski suggested changes May 8, 2019

View reviewed changes

Brord van Wierst added 2 commits May 10, 2019 14:54

Added comments, removing throws

87e1d0a

Merge branch 'dev' into ls-extra-unit-tests

4cff6bb

GalRogozinski approved these changes May 12, 2019

View reviewed changes

GalRogozinski merged commit 2b40917 into iotaledger:dev May 12, 2019

jakubcech mentioned this pull request May 27, 2019

Release 1.7.1 #1463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test: Local Snapshot unit tests #1364

Test: Local Snapshot unit tests #1364

kwek20 commented Feb 27, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

kwek20 Apr 29, 2019

GalRogozinski May 8, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski Apr 29, 2019

GalRogozinski May 8, 2019

GalRogozinski left a comment

GalRogozinski May 8, 2019

GalRogozinski May 8, 2019

GalRogozinski May 8, 2019

Test: Local Snapshot unit tests #1364

Test: Local Snapshot unit tests #1364

Conversation

kwek20 commented Feb 27, 2019

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment