-
Notifications
You must be signed in to change notification settings - Fork 13
FDB-593 add wipe unit tests. #199
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #199 +/- ##
===========================================
+ Coverage 72.62% 72.72% +0.10%
===========================================
Files 360 361 +1
Lines 21705 21924 +219
Branches 2244 2255 +11
===========================================
+ Hits 15764 15945 +181
- Misses 5941 5979 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tests/fdb/api/test_wipe.cc
Outdated
| using namespace eckit; | ||
|
|
||
| namespace { | ||
| void deldir(eckit::PathName& p) { |
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.
I'm sure I've seen this functionality elsewhere before. Is there not a helper function we should be using?
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.
being addressed in ecmwf/eckit#236
| namespace test { | ||
|
|
||
| CASE("Setup") { | ||
|
|
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.
This doesn't look right.
Test cases should be self contained (using an object as a fixture if needed).
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.
Fixed
|
|
||
| char data[] = "test"; | ||
|
|
||
| SECTION("WIPE MULTIPLE DATABASES; DRY-RUN WIPE") { |
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.
don't know why the label is in all-caps
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.
This is sometimes helpful to quickly see where the output for a given section starts or ends in the middle of long and cluttered test outputs.
tests/fdb/api/test_wipe.cc
Outdated
| // wipe one database | ||
| fdb5::WipeElement elem; | ||
| auto wipeObject = fdb.wipe(dbReq1, true); | ||
| while (wipeObject.next(elem)) |
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.
Always put {} for if and while if the contents are on a separate line.
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.
Manually fixed for now, but a discussion is taking place separately to potentially add this into clang-format.
tests/fdb/api/test_wipe.cc
Outdated
| // FDB configurations to test | ||
|
|
||
| std::string localSingleRootConfig{ | ||
| "spaces:\n" |
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.
Use C++ multi-line strings.
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
Description
Adds wipe unit tests.
Contributor Declaration
By opening this pull request, I affirm the following:
🌈🌦️📖🚧 Documentation Z3FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/z3fdb/pull-requests/PR-199
🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-199