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.

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

ci: Reduce test resource usage and test with file db #791

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #682

Description

Closes databases once use has finished, massively reducing peak memory and bypassing the issues seen when maxing out mem consumption.

Also changes the ci tests to run with badger IM and file, instead of just IM.

Related issue #684 is deemed out of scope.

How has this been tested?

Tested the db.Closing issue by hard coding the internal bools to always run with IM and file. Tested the ci environment vars by hardcoding the file db to panic on init.

@AndrewSisley AndrewSisley added area/testing Related to any test or testing suite ci/build This is issue is about the build or CI system, and the administration of it. code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels Sep 13, 2022
@AndrewSisley AndrewSisley requested a review from a team September 13, 2022 19:11
@AndrewSisley AndrewSisley self-assigned this Sep 13, 2022
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #791 (4734a37) into develop (d95e4cb) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #791      +/-   ##
===========================================
+ Coverage    59.11%   59.13%   +0.01%     
===========================================
  Files          153      153              
  Lines        16977    16977              
===========================================
+ Hits         10036    10039       +3     
+ Misses        6023     6021       -2     
+ Partials       918      917       -1     
Impacted Files Coverage Δ
datastore/badger/v3/datastore.go 38.79% <0.00%> (+0.48%) ⬆️

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

That was easy xD

LGTM

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.

Great fix! Thanks. Build rule is perfect like we discussed. Sorry about accidentally hitting the update branch button on my phone.

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.

👍

@@ -378,6 +378,8 @@ func ExecuteQueryTestCase(
assert.Fail(t, "Expected an error however none was raised.", test.Description)
}
}

dbi.db.Close(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤣

Makefile Outdated
@@ -110,6 +110,10 @@ clean\:test:
test:
gotestsum --format pkgname -- ./... -race -shuffle=on

.PHONY: test\:ci
test\:ci:
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true gotestsum --format pkgname -- ./... -race -shuffle=on
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Keep the verbosity as we had before? to show the name of each test.

Suggested change
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true gotestsum --format pkgname -- ./... -race -shuffle=on
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true gotestsum --format testname -- ./... -race -shuffle=on

Makefile Outdated
@@ -110,6 +110,10 @@ clean\:test:
test:
gotestsum --format pkgname -- ./... -race -shuffle=on

.PHONY: test\:ci
test\:ci:
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true gotestsum --format pkgname -- ./... -race -shuffle=on
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: here is another suggestion similar to the previous one but cleaner. As long as this works for MacOS too.

Suggested change
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true gotestsum --format pkgname -- ./... -race -shuffle=on
DEFRA_BADGER_MEMORY=true DEFRA_BADGER_FILE=true $(MAKE) test:names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cheers, will change

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AndrewSisley ping me when you push the change and I'll test on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done now :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

working 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks for checking pre-merge :)

@AndrewSisley AndrewSisley merged commit e1b663e into develop Sep 14, 2022
@AndrewSisley AndrewSisley deleted the sisley/fix/I682-im-file-tests branch September 14, 2022 14:23
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Close database instance once finished with it

* Run CI tests on IM and file databases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/testing Related to any test or testing suite ci/build This is issue is about the build or CI system, and the administration of it. code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flakey tests with DEFRA_BADGER_MEMORY and DEFRA_BADGER_FILE
4 participants