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

Consume Detours for function mocking #421

Merged
merged 38 commits into from
Jan 5, 2024
Merged

Consume Detours for function mocking #421

merged 38 commits into from
Jan 5, 2024

Conversation

dunhor
Copy link
Member

@dunhor dunhor commented Jan 2, 2024

A little background - internal to the Windows repo there's a library for mocking functions for the purpose of writing tests. As of this writing, this library has not yet been made public. The original tests for WIL (before being made open source) relied on this library for a number of things. When porting the tests to this repo, the tests that rely on this library either needed to be modified to use alternative means for testing the same functionality or omitted entirely. This led to a number of hacks to try and achieve the same functionality, some of which have historically caused issues.

This change is an attempt to replace the use of that library with something that uses the Detours library to achieve something similar (it's noteworthy that the internal library also uses Detours). This code isn't intended to be as feature rich as the internal library and is targeted at providing just what's necessary for our tests. In particular, there are two types used for mocking:

  • detoured_thread_function - detours a function, however it only invokes the callback if the call came in on the thread that created the object. This is to avoid unintended consequences of some other thread interfering with the test - e.g. because of running tests in parallel, random chance, etc.
  • detoured_global_function - detours a function that calls the function object regardless of which thread called the detoured function. This is intended as a last resort option when the thread making the call cannot be predicted - e.g. from a thread pool thread, etc.

Both of these types are designed to be able to be constructed with a lambda since testing function pointers + globals is often a mess. Additionally, both are designed such that calls are non-reentrant on the thread that called them. This is nice because it allows the detour to just invoke the function it's detouring by name as opposed to needing to do something like reference the object holding it, however this also means that some scenarios may not work 100% as expected (e.g. your detour => detoured function => other function... => detoured function).

I've updated relevant tests to use these types as opposed to the work arounds we've been using & added some tests that were never copied in the first place. Now, this change isn't without some negatives:

  1. In order to use Detours, the tests now have a dependency on vcpkg. This is an extra setup step - albeit one most people will only need to do once - however it does increase the "barrier to entry" slightly. Aside - I also took the opportunity to switch to consuming Catch2 via vcpkg while I was at it. NOTE: I'm unsure if this may have consequences for code consuming WIL from vcpkg.
  2. I bumped the minimum C++ standard version that the tests build against to C++17 because it was a huge pain to write the detoured function types without inline static variables. I don't think this will be a major issue - C++17 has been out for quite some time now and the Windows build switched to C++17 a while ago, however it's noteworthy that we no longer have build coverage.

Ultimately, these tradeoffs seem worth it to me; just calling them out to see what others think.

Fixes #393

@dunhor dunhor requested review from ChrisGuzak and jonwis January 2, 2024 22:37
{
return *any;
THROW_HR(E_NOT_SET);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that changes like this are a consequence of defining RESULT_NORETURN to nothing. I debated trying to change it so that only the failfast functions are marked with RESULT_NORETURN since those are the only ones that we detour, however this is simple enough

tests/StlTests.cpp Outdated Show resolved Hide resolved
@dunhor
Copy link
Member Author

dunhor commented Jan 2, 2024

Note that an alternative to adding a dependency on vcpkg is to instead check in & build Detours ourselves. Personally, I'm not a huge fan, but...

Copy link
Member

@ChrisGuzak ChrisGuzak left a comment

Choose a reason for hiding this comment

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

wow... great work Duncan.

vcpkg.json Show resolved Hide resolved
include/wil/registry.h Outdated Show resolved Hide resolved
tests/CoroutineTests.cpp Outdated Show resolved Hide resolved
vcpkg.json Show resolved Hide resolved
});
witest::detoured_thread_function<&::LocalFree> detour;
REQUIRE_SUCCEEDED(detour.reset([](HLOCAL p) -> HLOCAL {
HLOCAL h = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

wil::unique_hlocal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure that helps much here. h is the result of the call to LocalFree and is returned to the caller. We also couldn't use it for wrapping p since we need the result of the call to LocalFree to return to the caller.

tests/wiTest.cpp Outdated Show resolved Hide resolved
@dunhor dunhor merged commit b45b91e into master Jan 5, 2024
11 checks passed
@dunhor dunhor deleted the dunhor/detours branch January 5, 2024 04:55
@asklar
Copy link
Member

asklar commented Jan 5, 2024

hooray :)

@dunhor
Copy link
Member Author

dunhor commented Jan 5, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoRegisterMallocSpy causes unit tests to fail periodically
5 participants