-
Notifications
You must be signed in to change notification settings - Fork 203
chore: add INTERCEPT capability to enable unit test code to run arbitrary code at predefined points in the library code #5725
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
Conversation
|
To spread awareness I have added all core developers as reviewers. |
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.
We can log seemingly arbitrary details of an execution to assert that certain conditions are met by a user input
Why is (p|i)assert not sufficient?
tiledb/common/util/intercept.h
Outdated
| #define INTERCEPT(name, ...) \ | ||
| do { \ | ||
| ([](auto... args) { \ | ||
| name().event(std::forward<decltype(args)>(args)...); \ | ||
| })(__VA_ARGS__); \ | ||
| } while (0) |
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.
Can this be not a macro?
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.
The macro is useful for
#ifdef TILEDB_INTERCEPTS
#define INTERCEPT(name, ...) <etc>
#else
#define INTERCEPT(...)
#endif
I don't think I was clear. What I am thinking is something more analogous to code coverage than to an assert - i.e. does a certain input cause a particular event to happen. For example, with #5700 we want to observe that the read query thread waits for memory to become available if it does not have enough to buffer at least one cell. We can set up input which we expect to cause this to happen but we don't have a way to ask afterwards if it actually did. But we could place an |
kounelisagis
left a comment
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.
LGTM
bekadavis9
left a comment
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.
LGTM, thanks for adding this
This is a follow-up to #5725 which added the `INTERCEPT` capability to inject essentially arbitrary code into pre-defined points in the execution of the core library (when enabled by the feature, of course). This pull request uses this new capability to replace an older class whose goal was quite similar: the `UnitTestConfig`. This class was a singleton with some predefined parameters which could be set by unit tests and then checked in-line by the library code in order to simulate specific behaviors. Based on its broad inclusion in test sources, this class probably used to do more than it does now; as of this writing it has just one parameter which is used in one place. This is an excellent use case for interception since we can move the test parameter exclusively into the test code, with the library code just intercepting relevant variables from its stack frame. So, this pull request replaces the old with the new. --- TYPE: NO_HISTORY DESC: replace UnitTestConfig with INTERCEPT
This pull request adds an "interception" capability which allows us to run arbitrary code at pre-defined "interception points" which could be placed either in other unit test code, or within the library code itself.
Interception points can be used to strengthen our tests in several ways. We can log seemingly arbitrary details of an execution to assert that certain conditions are met by a user input; we can simulate errors occurring at specific points in an execution; we can synchronize multiple threads to ensure a deterministic outcome from multi-threaded code; and more.
This pull request adds macros
DECLARE_INTERCEPT,DEFINE_INTERCEPT, andINTERCEPTfor instrumenting code with interception points, and also add a unit test which demonstrates instrumenting a "library function" in each of the aforementioned ways.TYPE: NO_HISTORY
DESC: Add interception capability