rollbear / trompeloeil

Header only C++14 mocking framework
Boost Software License 1.0
802 stars 85 forks source link

[wip] add runtime ``TIMES`` #333

Open DNKpp opened 4 months ago

DNKpp commented 4 months ago

as discussed in this issue https://github.com/rollbear/trompeloeil/issues/283 it would be very helpful to have the TIMES action configurable with runtime arguments. This PR aims to add that, without making too much changes to existing symbols (in fact, no changes are made at all).

As this is mainly a sketchup for the actual feature, I'm open for feedback regarding behavior and naming (maybe ``RT_TIMES is a more appropriate name).

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.92%. Comparing base (97f8167) to head (2ddb70a). Report is 10 commits behind head on main.

:exclamation: Current head 2ddb70a differs from pull request most recent head 80475c3. Consider uploading reports for the commit 80475c3 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #333 +/- ## =========================================== + Coverage 34.87% 98.92% +64.04% =========================================== Files 10 11 +1 Lines 906 931 +25 =========================================== + Hits 316 921 +605 + Misses 590 10 -580 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

DNKpp commented 4 months ago

These detected memory leaks seem to be caused by the exception thrown from the dynamic_times constructor. But that's not the root cause of the issue, as trompeloeil is surprisingly very bad at managing some of its resources (e.g. the matcher is "forwarded" via raw pointer).

So, that is rather unrelated to that PR here, but we should definitely address that in another PR.

rollbear commented 4 months ago

From a very brief glance, this looks interesting. I'm away on a conference right now, but I'll have a closer look when I get back home again.

rollbear commented 3 months ago

This is looking really neat. Thank you. Is there something you want to discuss, given that it's still marked [WIP]?

Otherwise, I think it's just a matter of adding docs and negative test(s) under 'compilation_errors' before I can merge it.

Naming can always be discussed, but I think DYN_TIMES is as good as anything I can come up with.

rollbear commented 2 months ago

Ping! I know I haven't been very attentive lately, but I hope to be able to tag a release in the tear future, say during the weekend July 13-14th. If there are things you want from me to be able to merge this, then let me know. As mentioned before, I'm cool with the change and the names, but would appreciate docs and some more tests.

DNKpp commented 2 months ago

Hey, sry for the delay. I'm currently very busy with my thesis, so I won't have much spare time for this during the next month or so.

I'm not sure, which tests I should add, as I mostly reuse existing components and already added a couple of tests. The negative tests are in the section beginning at 4835. ~Sure, we could explicitly add some constexpr tests, which will not compile with invalid bounds (because of the exception),~ (no, we can not, as c++11 constexpr is very limited) but I don't think that would be of much value.

I would really like to rename it to RT_TIMES as this seems to me to be more in line with the rest of the framework (which already uses the 2 sign prefixes for other features).

rollbear commented 2 months ago

Ok, no worries. There's no hurry.