[bisq-network/bisq] PaymentAccount object removal from ObservableSet issue (#3572)

[Julian Knutsen]

julianknutsen requested changes on this pull request.

Test needs some work so it will fail before your patch. I've given an example of one inline.

> @@ -71,4 +76,24 @@ private static PaymentAccount createAccountWithAge(AccountAgeWitnessService serv
 
         return account;
     }
+
+    @Test
+    public void testRemoveAccount() {
+        PaymentAccountPayload payload = mock(PaymentAccountPayload.class);
+
+        PaymentAccount account = mock(PaymentAccount.class);
+        account.setAccountName("account name");
+        when(account.getPaymentAccountPayload()).thenReturn(payload);

The issue here is that using mock doesn't allow you to use instance variables. The PaymentAccount object actually looks like this after instantiation:

```
class PaymentAccount() {
  String id = null; 
  long creationDate = 0;

  PaymentAccountPayload  getPaymentAccountPayload() {
      return payload; // mock(PaymentAccountPayload.class)
  }
```

So the instance variable accesses in the Equals method are just going to return default values since that is the mockito behavior, and you can't actually set them. The best you can do is override getId() and getCreationDate() to return what you want AND also use those in the equals() method, but that is adding to much custom code just to get mockito to work.

The second issue is that you are passing in the same PaymentAccount instance to remove() so the lombok generated equals call is going to succeed with or without your patch.

This is a good example where just using real objects will do what you want. Build two objects that won’t pass the original equals() method, but will pass the version you added. The following fails without his patch and succeeds with it.

```
    @Test
    public void testRemoveAccount() {
        PaymentAccount firstAccount = new RevolutAccount();
        firstAccount.init();

        ObservableSet<PaymentAccount> paymentAccountsAsObservable;
        paymentAccountsAsObservable = FXCollections.observableSet(new UserPayload().getPaymentAccounts());

        //add element
        paymentAccountsAsObservable.add(firstAccount);

        //remove element with some state change
        PaymentAccount secondAccount = new RevolutAccount();
        secondAccount.init();
        secondAccount.setAccountName("new account name");
        secondAccount.setId(firstAccount.getId());
        secondAccount.setCreationDate(firstAccount.getCreationDate().getTime());

        assertTrue(paymentAccountsAsObservable.remove(secondAccount));
    }
```

> @@ -148,6 +149,20 @@ public TradeCurrency getSingleTradeCurrency() {
             return null;
     }
 
+    @Override

This seems to work even though the EqualsAndHashCode annotation is still on the class. But, if you are overriding them it may make sense to remove it so lombok doesn't generate the code or future readers assume the generate equals is valid for this class.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3572#pullrequestreview-314590780
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191109/2348e106/attachment.html>