refactor: Improve CLI with test suite and builder pattern #928

orpheuslummis · 2022-11-01T17:57:44Z

Relevant issue(s)

Resolves #593

Description

This introduces the builder pattern, unit tests, integration testing, and removes globals.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

CI

Specify the platform(s) on which this was tested:

MacOS

codecov · 2022-11-01T18:02:07Z

Codecov Report

Merging #928 (8cecd52) into develop (75f6da8) will increase coverage by 2.57%.
The diff coverage is 45.29%.

❗ Current head 8cecd52 differs from pull request most recent head 0983d4f. Consider uploading reports for the commit 0983d4f to get more accurate results

@@             Coverage Diff             @@
##           develop     #928      +/-   ##
===========================================
+ Coverage    70.57%   73.15%   +2.57%     
===========================================
  Files          185      185              
  Lines        17934    18674     +740     
===========================================
+ Hits         12657    13661    +1004     
+ Misses        4315     3976     -339     
- Partials       962     1037      +75

Impacted Files	Coverage Δ
config/errors.go	`43.75% <0.00%> (-1.42%)`	⬇️
cli/dump.go	`11.11% <11.90%> (+6.34%)`	⬆️
cli/p2p_collection_getall.go	`16.21% <17.14%> (+10.15%)`	⬆️
cli/p2p_collection_add.go	`20.00% <20.00%> (+12.30%)`	⬆️
cli/p2p_collection_remove.go	`20.00% <20.00%> (+12.30%)`	⬆️
cli/replicator_delete.go	`23.80% <23.72%> (+12.69%)`	⬆️
cli/replicator_set.go	`24.59% <23.72%> (+13.05%)`	⬆️
cli/schema_patch.go	`32.14% <25.28%> (+28.91%)`	⬆️
cli/schema_add.go	`29.72% <25.55%> (+26.57%)`	⬆️
cli/request.go	`36.73% <28.94%> (+34.32%)`	⬆️
... and 17 more

... and 20 files with indirect coverage changes

AndrewSisley

I gave this a quick look over, skimming over the function internals (will do once mentioned bugs are fixed etc) - I like the structural changes quite a lot and it looks like a nice change overall. Is much much nicer than relying on init calls.

I am a bit nervous about merging a refactor of the CLI code so much right before the release though, especially given that it is not very well tested compared to stuff like the GQL query system. But I hope we can get this in soon :)

jsimnz

Big fan of the new changes pattern. Will make things much nicer/easier moving forward.

Reviewing this PR highlighted how brittle/poor our client flow is for consuming. Really need to put together a proper HTTP and GRPC client package, that this can easily consume, instead defining/building everything inline.

But that is entirely independant of this PR work.

Leaving as a comment for now without explicit approve/request change, we can get this merged for 0.5.1 👍

cli/cli.go

jsimnz · 2023-04-05T21:47:14Z

cli/p2p_collection_remove.go

+			cred := insecure.NewCredentials()
+			client, err := netclient.NewClient(cfg.Net.RPCAddress, grpc.WithTransportCredentials(cred))
+			if err != nil {
+				return errors.Wrap("failed to create RPC client", err)
+			}
+
+			rpcTimeoutDuration, err := cfg.Net.RPCTimeoutDuration()
+			if err != nil {
+				return errors.Wrap("failed to parse RPC timeout duration", err)
+			}
+
+			ctx, cancel := context.WithTimeout(cmd.Context(), rpcTimeoutDuration)
+			defer cancel()


thought(out of scope): Just adding here to write it down. We should probably pull out the RPC client setup stuff to the root rpc command and pass the built client over the command.Context object. Will create a followup issue for this.

reminder of this, in case you still want to open the issue

islamaliev · 2023-04-14T08:32:38Z

tests/integration/cli/client_schema_add_test.go

+	assertContainsSubstring(t, stdout2, `schema type already exists. Name: Post`)
+}
+
+/* disabled because current implementation doesn't support this currently


todo: remove commented code

Not sure what convention Source is sticking to in terms of commented code but usually it's not a good practice

I also think this should be removed if not used.

islamaliev

Change look good, but I kinda missed the "builder pattern" part

jsimnz

LGTM

orpheuslummis · 2023-04-23T19:59:48Z

LGTM

Im rebasing, resolving conflicts, then I'll merge tomorrow

orpheuslummis · 2023-04-24T04:31:46Z

There was a few issues, related to the rootdir, and test commands to run a defradb node goroutine or to run a defra command, which I've fixed in the latest rebase. I suggest this has a last round of review before being merged.

fredcarle · 2023-04-24T22:48:45Z

There was a few issues, related to the rootdir, and test commands to run a defradb node goroutine or to run a defra command, which I've fixed in the latest rebase. I suggest this has a last round of review before being merged.

Is there a specific commit that has those?

orpheuslummis · 2023-04-24T23:32:02Z

There was a few issues, related to the rootdir, and test commands to run a defradb node goroutine or to run a defra command, which I've fixed in the latest rebase. I suggest this has a last round of review before being merged.

Is there a specific commit that has those?

this commit orpheuslummis@ec46190
and resolving some merge conflicts in earlier commits..

fredcarle

Looks good. Just a few comments that I'd suggest resolving before merge.

cli/root.go

tests/integration/cli/client_rpc_replicator_test.go

fredcarle · 2023-04-25T02:45:50Z

tests/integration/cli/client_schema_add_test.go

+	assertContainsSubstring(t, stdout2, `schema type already exists. Name: Post`)
+}
+
+/* disabled because current implementation doesn't support this currently


I also think this should be removed if not used.

orpheuslummis · 2023-04-25T02:48:53Z

I also think this should be removed if not used.

oops, I thought I had removed it.

AndrewSisley

LGTM, good work Orpheus :)

…ork#928)

orpheuslummis added area/testing Related to any test or testing suite area/cli Related to the CLI binary labels Nov 1, 2022

orpheuslummis added this to the DefraDB v0.4 milestone Nov 1, 2022

orpheuslummis self-assigned this Nov 1, 2022

orpheuslummis changed the title ~~refactor: Add test suite and builder pattern to CLI~~ refactor: Improve CLI with test suite and builder pattern Nov 4, 2022

orpheuslummis added the action/no-benchmark Skips the action that runs the benchmark. label Nov 4, 2022

sourcenetwork deleted a comment from source-devs Nov 4, 2022

orpheuslummis force-pushed the orpheus/refactor/cli-builder-pattern-and-test-suite branch from bacf80f to 6334854 Compare December 16, 2022 17:41

jsimnz force-pushed the develop branch from a615303 to d87bae8 Compare January 8, 2023 00:16

jsimnz modified the milestones: DefraDB v0.4, DefraDB v0.5 Jan 20, 2023

orpheuslummis force-pushed the orpheus/refactor/cli-builder-pattern-and-test-suite branch from 6334854 to 9be4ae4 Compare March 23, 2023 01:31

orpheuslummis mentioned this pull request Mar 29, 2023

feat: Add SchemaPatch CLI command #1250

Merged

4 tasks

orpheuslummis force-pushed the orpheus/refactor/cli-builder-pattern-and-test-suite branch 2 times, most recently from 73dbdbf to 5adb176 Compare March 30, 2023 19:49

orpheuslummis marked this pull request as ready for review March 30, 2023 19:52

AndrewSisley reviewed Mar 31, 2023

View reviewed changes

jsimnz modified the milestones: DefraDB v0.5, DefraDB v0.5.1 Apr 5, 2023

jsimnz added the refactor This issue specific to or requires *notable* refactoring of existing codebases and components label Apr 5, 2023

jsimnz reviewed Apr 5, 2023

View reviewed changes

islamaliev reviewed Apr 14, 2023

View reviewed changes

fredcarle mentioned this pull request Apr 23, 2023

Add configuration for allowed origins #1355

Closed

jsimnz approved these changes Apr 23, 2023

View reviewed changes

orpheuslummis force-pushed the orpheus/refactor/cli-builder-pattern-and-test-suite branch from 5dfb279 to 226ab35 Compare April 24, 2023 04:23

fredcarle approved these changes Apr 25, 2023

View reviewed changes

AndrewSisley approved these changes Apr 25, 2023

View reviewed changes

orpheuslummis added 6 commits April 25, 2023 14:14

builder pattern

b0ca0d6

cli unit tests

7090816

update programs in cmd directory

5fa3399

config is updated too

12ec2af

cli integration tests

6855dd0

apply feedback and fixes

0983d4f

orpheuslummis force-pushed the orpheus/refactor/cli-builder-pattern-and-test-suite branch from 8cecd52 to 0983d4f Compare April 25, 2023 18:15

orpheuslummis merged commit df0d323 into develop Apr 25, 2023
13 checks passed

orpheuslummis deleted the orpheus/refactor/cli-builder-pattern-and-test-suite branch April 25, 2023 18:24

shahzadlone mentioned this pull request Jun 27, 2023

CLI tests failure due to data race #1594

Closed

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

refactor: Improve CLI with test suite and builder pattern (sourcenetw…

7d2ce6e

…ork#928)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Improve CLI with test suite and builder pattern #928

refactor: Improve CLI with test suite and builder pattern #928

orpheuslummis commented Nov 1, 2022 •

edited

codecov bot commented Nov 1, 2022 •

edited

AndrewSisley left a comment

jsimnz left a comment

jsimnz Apr 5, 2023

orpheuslummis Apr 23, 2023

islamaliev Apr 14, 2023

fredcarle Apr 25, 2023

islamaliev left a comment

jsimnz left a comment

orpheuslummis commented Apr 23, 2023

orpheuslummis commented Apr 24, 2023

fredcarle commented Apr 24, 2023

orpheuslummis commented Apr 24, 2023 •

edited

fredcarle left a comment

fredcarle Apr 25, 2023

orpheuslummis commented Apr 25, 2023 •

edited

AndrewSisley left a comment

refactor: Improve CLI with test suite and builder pattern #928

refactor: Improve CLI with test suite and builder pattern #928

Conversation

orpheuslummis commented Nov 1, 2022 • edited

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Nov 1, 2022 • edited

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

jsimnz Apr 5, 2023

Choose a reason for hiding this comment

orpheuslummis Apr 23, 2023

Choose a reason for hiding this comment

islamaliev Apr 14, 2023

Choose a reason for hiding this comment

fredcarle Apr 25, 2023

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

orpheuslummis commented Apr 23, 2023

orpheuslummis commented Apr 24, 2023

fredcarle commented Apr 24, 2023

orpheuslummis commented Apr 24, 2023 • edited

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Apr 25, 2023

Choose a reason for hiding this comment

orpheuslummis commented Apr 25, 2023 • edited

AndrewSisley left a comment

Choose a reason for hiding this comment

orpheuslummis commented Nov 1, 2022 •

edited

codecov bot commented Nov 1, 2022 •

edited

orpheuslummis commented Apr 24, 2023 •

edited

orpheuslummis commented Apr 25, 2023 •

edited