-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cheers, will change
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working 👍
There was a problem hiding this comment.
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 :)
d591e94
to
49aff4c
Compare
49aff4c
to
4734a37
Compare
* Close database instance once finished with it * Run CI tests on IM and file databases
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.