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

fix: Standardise schema migration CLI errors #1682

Merged
merged 8 commits into from
Jul 21, 2023
Merged

fix: Standardise schema migration CLI errors #1682

merged 8 commits into from
Jul 21, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1652

Description

Standardises schema migration set errors. And errors if given unknown lens cfg properties.

Adds integration tests for the command.

@AndrewSisley AndrewSisley added code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels Jul 21, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.6 milestone Jul 21, 2023
@AndrewSisley AndrewSisley requested a review from a team July 21, 2023 20:19
@AndrewSisley AndrewSisley self-assigned this Jul 21, 2023
@AndrewSisley AndrewSisley changed the title fix: Standardise schema migration set errors fix: Standardise schema migration set errors Jul 21, 2023
@AndrewSisley AndrewSisley force-pushed the 1673-cli-lens-capitalization branch from 3033df6 to 8cc57f0 Compare July 21, 2023 20:24
@codecov
Copy link

codecov bot commented Jul 21, 2023 •
edited
Loading

Codecov Report

Patch coverage: 94.44% and project coverage change: +0.53 🎉

Comparison is base (4eb0009) 74.89% compared to head (0e86ebd) 75.43%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1682      +/-   ##
===========================================
+ Coverage    74.89%   75.43%   +0.53%     
===========================================
  Files          203      203              
  Lines        21091    21092       +1     
===========================================
+ Hits         15796    15909     +113     
+ Misses        4214     4086     -128     
- Partials      1081     1097      +16
Flag Coverage Δ
all-tests 75.43% <94.44%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cli/schema_patch.go 31.90% <0.00%> (ø)
cli/errors.go 25.00% <100.00%> (+11.67%) ⬆️
cli/schema_migration_get.go 38.33% <100.00%> (+20.59%) ⬆️
cli/schema_migration_set.go 54.14% <100.00%> (+39.74%) ⬆️

... and 11 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4eb0009...0e86ebd. Read the comment docs.

@AndrewSisley AndrewSisley force-pushed the 1673-cli-lens-capitalization branch from 8cc57f0 to d95f630 Compare July 21, 2023 20:38
@AndrewSisley AndrewSisley force-pushed the 1673-cli-lens-capitalization branch from d95f630 to 8ce5a42 Compare July 21, 2023 20:40
@AndrewSisley AndrewSisley changed the title fix: Standardise schema migration set errors fix: Standardise schema migration CLI errors Jul 21, 2023
islamaliev
islamaliev approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

Looks good Andy. I'm not sure if this should be part of this PR but would it be possible to add integration tests with invalid arguments to a lens module?

{"lenses": [{"path": "/path/to/remove.wasm", "arguments": {"invalid":"verified"}}]}

@AndrewSisley
Copy link
Contributor Author

AndrewSisley commented Jul 21, 2023 •
edited
Loading

Looks good Andy. I'm not sure if this should be part of this PR but would it be possible to add integration tests with invalid arguments to a lens module?

{"lenses": [{"path": "/path/to/remove.wasm", "arguments": {"invalid":"verified"}}]}

There is one such test - TestSchemaMigrationSet_GivenCfgWithLenseError_ShouldError - I see further testing of that as outside the scope of the CLI tests - they should (and are) instead tested within the lens repo and (a little bit) in the core defra integration tests.

Within the CLI tests for Lens I just care that it does report wasm generated errors to the user, I see the specifics regarding which errors and how/when they are generated as out of scope here.

shahzadlone
shahzadlone approved these changes Jul 21, 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

Makefile
@@ -29,7 +29,7 @@ ifdef BUILD_TAGS
BUILD_FLAGS+=-tags $(BUILD_TAGS)
endif

TEST_FLAGS=-race -shuffle=on -timeout 70s
TEST_FLAGS=-race -shuffle=on -timeout 120s
Copy link
Member

Choose a reason for hiding this comment

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

Question: what caused this bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More tests == even slower :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess a bunch of CLI tests

@AndrewSisley AndrewSisley merged commit eda250a into sourcenetwork:develop Jul 21, 2023
@AndrewSisley AndrewSisley deleted the 1673-cli-lens-capitalization branch July 21, 2023 21:45
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
fix: Standardise schema migration CLI errors (sourcenetwork#1682)
c44d457 
## Relevant issue(s)

Resolves sourcenetwork#1652

## Description

Standardises `schema migration` CLI errors. And errors if given unknown
lens cfg properties.

Adds integration tests for the commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@islamaliev islamaliev islamaliev approved these changes

@shahzadlone shahzadlone shahzadlone approved these changes

Assignees

@AndrewSisley AndrewSisley

Labels
action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary code quality Related to improving code quality
Projects
None yet
Milestone
DefraDB v0.6
Development

Successfully merging this pull request may close these issues.

Add tests for schema migration CLI commands
3 participants
@AndrewSisley @islamaliev @shahzadlone