chore(tests): improve reactions tests using go mock #1091

dadamu · 2023-03-10T13:40:26Z

Description

Closes: #XXXX
This PR improves the x/reactions tests using gomock.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2023-03-10T13:47:59Z

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.10 🎉

Comparison is base (ed8b761) 80.76% compared to head (221b247) 80.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   80.76%   80.86%   +0.10%     
==========================================
  Files         185      186       +1     
  Lines       16618    16659      +41     
==========================================
+ Hits        13421    13472      +51     
+ Misses       2610     2604       -6     
+ Partials      587      583       -4

Impacted Files	Coverage Δ
x/reactions/types/models.go	`77.48% <ø> (ø)`
x/posts/keeper/polls.go	`97.70% <90.00%> (-2.30%)`	⬇️
x/reactions/legacy/v3/store.go	`91.42% <91.42%> (ø)`
x/posts/abci.go	`100.00% <100.00%> (+6.25%)`	⬆️
x/posts/keeper/attachments.go	`100.00% <100.00%> (ø)`
x/reactions/keeper/alias_functions.go	`77.16% <100.00%> (+3.93%)`	⬆️
x/subspaces/types/permissions.go	`77.77% <100.00%> (ø)`

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dadamu · 2023-03-10T14:00:23Z

x/posts/keeper/polls.go

+	if !isTally {
+		k.AfterPollAnswerDeleted(ctx, subspaceID, postID, pollID, user)
+	}


AfterPollAnswerDeleted should not be called if the deletion is from tally operation.

Why was this decided? I believe we can call it safely in every occasion: if we are removing a user answer from the store, we should notify it anyway. It will be the listener's job to decide whether to handle it or not. And since the tally emits a custom event, it should be easy enough to distinguish the two cases if the listener wants

@dadamu I only have this comment left. Once this is solved, we can merge this

From my side, we can avoid from calling redundant AfterPollAnswerDeleted hooks since AfterPollVotingPeriodEnded handles the everything.

For example, when calling AfterPollAnswerDeleted, the listener can remove a user answer, like DELETE FROM user_answer WHERE user_address = "cosmos....". In the other hand, AfterPollVotingPeriodEnded can remove all the answers with DELETE FROM user_answer WHERE poll_id = 1.

Additionally, it is better performance if the listener module wants to handle the tallied poll only and no executions with answers.

The listener module handling the tallied poll only and no execution with answers in the current way will be like:

func (h Hooks) AfterPollAnswerDeleted(sdk.Context, subspaceID uint64, postID uint64, pollID uint32, user string) { if !IsTalliedPoll(ctx, subspaceID, postID, pollID) { handleAnswer(ctx, subspaceID, postID, pollID) } }

We need to check the poll is tallied or not in every answer deleted action, which costs O(N) redundantly where N = the amount of answers in the poll.

Ok, it makes sense. Then I would ask you if we could change one thing: instead of passing isTally to this function, which is only used to emit the event to the listener, can we instead move the call of k.AfterPollAnswerDeleted(ctx, subspaceID, postID, pollID, user) inside the Tally function direclty? This way we don't have to edit the signature of DeleteUserAnswer and we can keep it as it was before.

So, we remove this code from DeleteUserAnswer:

if !isTally { k.AfterPollAnswerDeleted(ctx, subspaceID, postID, pollID, user) }

And we edit the Tally function to have the following lines inside the IteratePollUserAnswers:

k.DeleteUserAnswer(ctx, answer.SubspaceID, answer.PostID, answer.PollID, answer.User, true) k.AfterPollAnswerDeleted(ctx, answer.SubspaceID, answer.PostID, answer.PollID, answer.User)

@RiccardoM Tally function should not call AfterPollAnswerDeleted from my side. The main thing is to avoid passing isTally to DeleteUserAnswer, right?
If so, I think the best way is to have a new function deleteUserAnswer only deletes user answer inside database.

@dadamu I'm honestly not convinced of this change to remove the call of AfterPollAnswerDeleted from DeleteUserAnswer. I think it's not logically correct: we are removing a user answer, so why not emit the event? Also, I think this is out of scope for this PR. I would like to ask you to revert this change and open another PR where we can have a discussion about this change without blocking this PR (that should only be related to tests and not breaking changes - like this one)

Agree with you, will open another PR. Reverted.

dadamu · 2023-03-10T14:04:13Z

x/posts/keeper/polls.go

+// EndPoll tallies the poll then update it inside storage
+func (k Keeper) EndPoll(ctx sdk.Context, poll types.Attachment) {
+	// Compute the poll results
+	results := k.Tally(ctx, poll.SubspaceID, poll.PostID, poll.ID)
+
+	// Update the content with the results
+	content := poll.Content.GetCachedValue().(*types.Poll)
+	content.FinalTallyResults = results
+
+	contentAny, err := codectypes.NewAnyWithValue(content)
+	if err != nil {
+		panic(err)
+	}
+	poll.Content = contentAny
+
+	k.SaveAttachment(ctx, poll)
+	k.RemoveFromActivePollQueue(ctx, poll)
+
+	k.AfterPollVotingPeriodEnded(ctx, poll.SubspaceID, poll.PostID, poll.ID)
+}


To allow to test AfterPollVotingPeriodEnded inside the keeper, it is moved here from abci.

dadamu · 2023-03-10T14:05:12Z

x/posts/keeper/hooks_test.go

+			suite.Require().True(hooks.CalledMap["AfterPollVotingPeriodEnded"])
+			suite.Require().False(hooks.CalledMap["AfterPollAnswerDeleted"])


AfterPollVotingPeriodEnded should not call AfterPollAnswerDeleted.

dadamu · 2023-03-10T14:05:34Z

x/posts/keeper/attachments.go

 		}
 	}
+
+	k.AfterAttachmentDeleted(ctx, subspaceID, postID, attachmentID)


Added since it was missed.

dadamu · 2023-03-10T14:07:19Z

x/posts/abci_test.go

-	sk := subspaceskeeper.NewKeeper(cdc, keys[subspacestypes.StoreKey], nil, nil)
-	rk := relationshipskeeper.NewKeeper(cdc, keys[relationshipstypes.StoreKey], sk)
-	authKeeper := authkeeper.NewAccountKeeper(cdc, keys[authtypes.StoreKey], pk.Subspace(authtypes.ModuleName), authtypes.ProtoBaseAccount, app.GetMaccPerms())
-	profilesKeeper := profileskeeper.NewKeeper(cdc, legacyAmino, keys[profilestypes.StoreKey], pk.Subspace(profilestypes.DefaultParamsSpace), authKeeper, rk, nil, nil, nil)
-	keeper := postskeeper.NewKeeper(cdc, keys[poststypes.StoreKey], pk.Subspace(types.DefaultParamsSpace), profilesKeeper, sk, rk)
+
+	keeper := postskeeper.NewKeeper(cdc, keys[poststypes.StoreKey], pk.Subspace(types.DefaultParamsSpace), nil, nil, nil)


Removed since abci test does not depend on these module.

dadamu · 2023-03-10T16:08:55Z

x/reactions/keeper/alias_functions.go

@@ -172,7 +172,7 @@ func (k Keeper) IteratePostReactions(ctx sdk.Context, subspaceID uint64, postID
 func (k Keeper) HasReacted(ctx sdk.Context, subspaceID uint64, postID uint64, user string, value types.ReactionValue) bool {
 	found := false
 	k.IteratePostReactions(ctx, subspaceID, postID, func(reaction types.Reaction) (stop bool) {
-		if reaction.Author == user && reaction.Value.GetCachedValue().(types.ReactionValue) == value {
+		if reaction.Author == user && reaction.Value.GetCachedValue().(types.ReactionValue).Equal(value) {


Using "==" always returns false here since types.ReactionValue is a interface

Does this fix mean we might have double reactions stored on chain? Should be create a migration to make sure those are deleted if that's the case?

It is, added the migration scripts and its test.

dadamu · 2023-03-10T16:09:31Z

x/reactions/keeper/alias_functions_test.go

@@ -116,7 +116,24 @@ func (suite *KeeperTestSuite) TestKeeper_HasReacted() {
 			postID:     1,
 			user:       "cosmos1efa8l9h4p6hmkps6vk8lu7nxydr46npr8qtg5f",
 			value:      types.NewRegisteredReactionValue(1),
-			expResult:  false,
+			expResult:  true,


Fix to true

manu0466

LGTM!

RiccardoM · 2023-03-13T15:03:33Z

@dadamu Would you mind fixing the conflicts that are now present please?

…ove-reactions-tests

dadamu · 2023-03-14T07:14:59Z

@RiccardoM Fixed

RiccardoM · 2023-03-14T17:29:52Z

x/reactions/legacy/v3/store.go

+		reactions = append(reactions, reaction)
+
+		if containsDuplicatedReaction(reactions, reaction) {
+			duplicatedReactions = append(duplicatedReactions, reaction)
+		}


I believe this can be done in another way that might make the code more readable:

if contains(reactions, reaction) { duplicatedReactions = append(duplicatedReactions, reaction) } reactions = append(reactions, reacion)

What do you think?

Agree with it, improved

Closes: #XXXX This PR improves the x/reactions tests using gomock. --- *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [ ] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

chore(tests): improve relationships tests using gomock

d4267ab

dadamu requested a review from a team as a code owner March 10, 2023 13:40

dadamu commented Mar 10, 2023

View reviewed changes

chore: improve x/posts hooks tests

523c1cd

dadamu force-pushed the paul/improve-reactions-tests branch from 10d1b96 to 523c1cd Compare March 10, 2023 14:01

dadamu commented Mar 10, 2023

View reviewed changes

dadamu added 2 commits March 10, 2023 22:08

chore: fix typo naming

27f0b24

fix: fix HasReacted function

a08d195

dadamu force-pushed the paul/improve-reactions-tests branch from 9752f01 to a08d195 Compare March 10, 2023 16:07

dadamu commented Mar 10, 2023

View reviewed changes

dadamu added 2 commits March 11, 2023 00:44

chore: add missing tests

ce0517b

chore: add migration script

bd05a7c

dadamu force-pushed the paul/improve-reactions-tests branch from 6809244 to bd05a7c Compare March 13, 2023 11:17

dadamu requested a review from RiccardoM March 13, 2023 11:17

chore: update changeset entry

afad8ec

manu0466 approved these changes Mar 13, 2023

View reviewed changes

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Mar 13, 2023

Merge branch 'master' of github.com:desmos-labs/desmos into paul/impr…

47830a1

…ove-reactions-tests

RiccardoM reviewed Mar 14, 2023

View reviewed changes

chore: apply suggestions

f2029b4

dadamu requested a review from RiccardoM March 15, 2023 09:02

dadamu added 2 commits March 22, 2023 12:37

chore: improve DeleteUserAnswer function

e16496d

revert: rollback DeleteUserAnswer

221b247

dadamu force-pushed the paul/improve-reactions-tests branch from c757146 to 221b247 Compare March 22, 2023 15:11

RiccardoM approved these changes Mar 22, 2023

View reviewed changes

Merge branch 'master' into paul/improve-reactions-tests

2683f89

mergify bot merged commit 1b17122 into master Mar 22, 2023

mergify bot deleted the paul/improve-reactions-tests branch March 22, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(tests): improve reactions tests using go mock #1091

chore(tests): improve reactions tests using go mock #1091

dadamu commented Mar 10, 2023

codecov bot commented Mar 10, 2023 •

edited

dadamu Mar 10, 2023

RiccardoM Mar 14, 2023

RiccardoM Mar 17, 2023

dadamu Mar 20, 2023

dadamu Mar 20, 2023

RiccardoM Mar 20, 2023 •

edited

dadamu Mar 22, 2023 •

edited

RiccardoM Mar 22, 2023 •

edited

dadamu Mar 22, 2023 •

edited

dadamu Mar 10, 2023

dadamu Mar 10, 2023

dadamu Mar 10, 2023

dadamu Mar 10, 2023

dadamu Mar 10, 2023

RiccardoM Mar 11, 2023

dadamu Mar 13, 2023

dadamu Mar 10, 2023

manu0466 left a comment

RiccardoM commented Mar 13, 2023

dadamu commented Mar 14, 2023

RiccardoM Mar 14, 2023

dadamu Mar 15, 2023

		suite.Require().True(hooks.CalledMap["AfterPollVotingPeriodEnded"])
		suite.Require().False(hooks.CalledMap["AfterPollAnswerDeleted"])

chore(tests): improve reactions tests using go mock #1091

chore(tests): improve reactions tests using go mock #1091

Conversation

dadamu commented Mar 10, 2023

Description

Author Checklist

Reviewers Checklist

codecov bot commented Mar 10, 2023 • edited

Codecov Report

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

RiccardoM Mar 20, 2023 • edited

Choose a reason for hiding this comment

dadamu Mar 22, 2023 • edited

Choose a reason for hiding this comment

RiccardoM Mar 22, 2023 • edited

Choose a reason for hiding this comment

dadamu Mar 22, 2023 • edited

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

manu0466 left a comment

Choose a reason for hiding this comment

RiccardoM commented Mar 13, 2023

dadamu commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2023 •

edited

RiccardoM Mar 20, 2023 •

edited

dadamu Mar 22, 2023 •

edited

RiccardoM Mar 22, 2023 •

edited

dadamu Mar 22, 2023 •

edited