Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: Boost test coverage for collection_update #1050

Merged
merged 5 commits into from
Jan 20, 2023
Merged

test: Boost test coverage for collection_update #1050

merged 5 commits into from
Jan 20, 2023

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Jan 20, 2023 •
edited
Loading

Relevant issue(s)

Resolves #1044

Description

Boosts test coverage for collection_update. Does this mostly via the removal of dead code, but I also added a couple of 'quick win' tests. There are still gaps (e.g. UpdateWith), but it should be better.

Codecov is not uploading (maybe as CI pushed the branch before PR opened), reduces the number of misses from 143 to 72.

@AndrewSisley AndrewSisley added area/collections Related to the collections system area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. labels Jan 20, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5 milestone Jan 20, 2023
@AndrewSisley AndrewSisley requested a review from a team January 20, 2023 20:56
@AndrewSisley AndrewSisley self-assigned this Jan 20, 2023
@AndrewSisley AndrewSisley force-pushed the sisley/chore/I1044-rm-dead-col-update-code branch from dedb3db to 0f56f36 Compare January 20, 2023 21:06
@codecov
Copy link

codecov bot commented Jan 20, 2023 •
edited
Loading

Codecov Report

Merging #1050 (19ac3e1) into develop (2eb7180) will increase coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1050      +/-   ##
===========================================
+ Coverage    67.90%   68.18%   +0.27%     
===========================================
  Files          171      171              
  Lines        16275    16211      -64     
===========================================
+ Hits         11052    11053       +1     
+ Misses        4298     4235      -63     
+ Partials       925      923       -2
Impacted Files Coverage Δ
db/collection_update.go 72.17% <ø> (+13.15%) ⬆️
net/client.go 78.04% <0.00%> (-7.32%) ⬇️
net/peer.go 48.32% <0.00%> (-2.16%) ⬇️
net/server.go 59.38% <0.00%> (+1.14%) ⬆️

fredcarle
fredcarle approved these changes Jan 20, 2023
View reviewed changes
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If we want those functions back in the future we can always refer to this PR's commit.

shahzadlone
shahzadlone reviewed Jan 20, 2023
View reviewed changes
db/collection_update.go
@@ -551,15 +500,6 @@ func getNillableArray[T any](
return arr, nil
}

func (c *collection) applyMergePatchOp( //nolint:unused
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @jsimnz wanted to keep some of these, not sure if his reason is still relevant.

#63 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing particularly complex has been removed (and it is not tested and possibly already broken). It is also very easy to get them back, and probably less expensive than keeping them around as dead code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't be sad John. Your code is safe in the git history 🤗

shahzadlone
shahzadlone approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but for the removal would double check with @jsimnz as I think he wanted to keep some of the dead code.

Some of that was decided here: #63

@AndrewSisley
Copy link
Contributor Author

LGTM, but for the removal would double check with @jsimnz as I think he wanted to keep some of the dead code.

Some of that was decided here: #63

The fact that they have been dead for more than a year, and have cost us time at least twice now strengthens my desire to remove them.

@AndrewSisley AndrewSisley force-pushed the sisley/chore/I1044-rm-dead-col-update-code branch from 0f56f36 to 19ac3e1 Compare January 20, 2023 21:28
@fredcarle fredcarle mentioned this pull request Jan 20, 2023
Open
@fredcarle
Copy link
Collaborator

LGTM, but for the removal would double check with @jsimnz as I think he wanted to keep some of the dead code.
Some of that was decided here: #63

The fact that they have been dead for more than a year, and have cost us time at least twice now strengthens my desire to remove them.

I've mentioned this PR in the JSON Patch issue. If we need it we will be able to find it easily.

@AndrewSisley AndrewSisley merged commit 465ca1d into develop Jan 20, 2023
@AndrewSisley AndrewSisley deleted the sisley/chore/I1044-rm-dead-col-update-code branch January 20, 2023 21:46
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
@AndrewSisley @shahzadlone
test: Boost test coverage for collection_update (#1050)
c732eb2 
* Remove unused functions from collection_update

* Remove duplicate if

Is done a couple of lines up, within the switch

* Add test to cover empty update filter

* Add test that updates a boolean

Oddly this was only supported type that we didnt already cover

* Add test for invalid update filter type
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
test: Boost test coverage for collection_update (sourcenetwork#1050)
44e8d17 
* Remove unused functions from collection_update

* Remove duplicate if

Is done a couple of lines up, within the switch

* Add test to cover empty update filter

* Add test that updates a boolean

Oddly this was only supported type that we didnt already cover

* Add test for invalid update filter type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimnz jsimnz jsimnz left review comments

@fredcarle fredcarle fredcarle approved these changes

@shahzadlone shahzadlone shahzadlone approved these changes

Assignees

@AndrewSisley AndrewSisley

Labels
action/no-benchmark Skips the action that runs the benchmark. area/collections Related to the collections system area/testing Related to any test or testing suite
Projects
None yet
Milestone
DefraDB v0.5
Development

Successfully merging this pull request may close these issues.

Low test coverage of collection update functions
4 participants
@AndrewSisley @fredcarle @jsimnz @shahzadlone