Add overloads to show additional custom message on failure#3
Conversation
|
Added a proposal of a message supplier based on string streams. Example usage: Assert::That(
shortestDistance(1, 2, 3),
EqualsWithDelta(4.242640687, 1e-9),
MessageBuilder("Invalid result for input: x=") << 1 << ", y=" << 2 << ", z=" << 3
); |
|
Added remaining overloads. Now every original overload of Obstacles: Default attributes Known problems: Overload for Example usage// #include "preloaded.h" if preloaded
// [solution]
#include <igloo/igloo_alt.h>
using namespace igloo;
// [tests]
#include <cmath>
#include <utility>
#include <cstdlib>
#include <ctime>
Describe(Overloads_And_Compatibility)
{
It(CompatTest_With_Fluent_Expression_Without_Message)
{
Assert::That(3.5, Is().EqualToWithDelta(3.6, 1e-9));
}
It(Test_With_Fluent_Expression_And_Message)
{
Assert::That(3.5, Is().EqualToWithDelta(3.6, 1e-9), AssertionMessage("Invalid result for input: 3.5"));
}
It(CompatTest_With_StringLiteral_Fluent_Expression_Without_Message)
{
Assert::That("a", Is().StartingWith("b"));
}
It(Test_With_StringLiteral_Fluent_Expression_And_Message)
{
Assert::That("a", Is().StartingWith("b"), AssertionMessage("Invalid result for input: a"));
}
It(CompatTest_With_Expression_Without_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9));
}
It(Test_With_Expression_And_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), AssertionMessage("Invalid result for input: 3.5"));
}
It(CompatTest_With_StringLiteral_Expression_Without_Message)
{
Assert::That("a", StartsWith("b"));
}
It(Test_With_StringLiteral_Expression_And_Message)
{
Assert::That("a", StartsWith("b"), AssertionMessage("Invalid result for input: a"));
}
It(CompatTest_With_Boolean_Without_Message)
{
Assert::That(false);
}
//does not compile
//It(Test_With_Boolean_And_Message)
//{
// Assert::That(false, AssertionMessage("Wrong answer!"));
//}
};
Describe(MessageSuppliers) {
Describe(Test_EmptyMessageSupplier) {
It(Test_With_Empty_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), EmptyMessageSupplier());
}
};
Describe(Test_StaticMessageSupplier) {
It(Test_With_Static_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), AssertionMessage("Invalid result for input: 3.5"));
}
};
Describe(Test_StreamMessageBuilder) {
It(Test_With_Message_Builder_And_Message_Prefix)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), MessageBuilder("Invalid result for input: ") << 3.5);
}
It(Test_With_Message_Builder_No_Message_Prefix)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), MessageBuilder() << 3.5 << " returned incorrect answer:");
}
};
Describe(Test_LazyEvaluationSupplier) {
It(Test_LazySupplier_Lambda) {
for (int input = 0; input < 100; ++input) {
//Potentially expensive way to build an assertion message.
//Postpone it until actually needed, do not calculate the message
//if assertion passes.
auto makemsg = [input]() -> std::string {
return std::string("Invalid answer for ") + std::string(input, '#');
};
Assert::That(input, IsLessThan(50), makemsg);
}
}
};
};
Describe(Booleans) {
It(With_ExpressionBuilder_No_Message) {
Assert::That(true, Is().False());
}
It(With_ExpressionBuilder_And_Message) {
Assert::That(true, Is().False(), AssertionMessage("Wrong answer!"));
}
It(With_Expression_No_Message) {
Assert::That(true, IsFalse());
}
It(With_Expression_And_Message) {
Assert::That(true, IsFalse(), AssertionMessage("Wrong answer!"));
}
It(Literal) {
Assert::That(false);
}
};
// [main]
#include <igloo/CodewarsTestListener.h>
int main(int, const char* []) {
NullTestResultsOutput output;
TestRunner runner(output);
CodewarsTestListener listener;
runner.AddListener(&listener);
runner.Run();
}Example output |
|
Initial thoughts. I don't think we should or need to provide much. Adding overloads that accept a callable returning a string to specify a custom message, and providing a convenient way to specify a string should be enough. I'd remove Assert::That(
3.5,
EqualsWithDelta(3.6, 1e-9),
MessageBuilder("Invalid result for input: ") << 3.5
);
// vs
Assert::That(
3.5,
EqualsWithDelta(3.6, 1e-9),
AssertionMessage(fmt::format("Invalid result for input: {}", 3.5))
);Assert::That(
3.5,
EqualsWithDelta(3.6, 1e-9),
MessageBuilder() << 3.5 << " returned incorrect answer"
);
// vs
Assert::That(
3.5,
EqualsWithDelta(3.6, 1e-9),
AssertionMessage(fmt::format("{} returned incorrect answer", 3.5))
);I'd also use Instead of having auto AssertionMessage = [](const std::string& msg) { return [&]() { return msg; }; };Might want a better name for |
I created the stream-based message supplier thinking about people like me, who have no idea about
I was considering making it a functor private to the library, but finally I decided to make it public. I thought that some users might want to have it, to use new overloads but keep the old behavior, or to switch implementations. Not that I have any specific use case for this. After all, you have things like Additionally, I used
Sure. I went with a functor for a couple of reasons:
But it's time to grow up, and lambdas got better support from VC++ nowadays, so why not ;)
I am somewhat concerned by ambiguity of |
- EmptyMessageSupplier and MessageStringSupplier changed from functors to lambdas - Removed MessageStreamSupplier - Renamed AssertionMessage to WithMessage
|
Per suggestions:
Example usage// #include "preloaded.h" if preloaded
// [solution]
#include <igloo/igloo_alt.h>
using namespace igloo;
// [tests]
#include <cmath>
#include <utility>
#include <cstdlib>
#include <ctime>
Describe(Overloads_And_Compatibility)
{
It(CompatTest_With_Fluent_Expression_Without_Message)
{
Assert::That(3.5, Is().EqualToWithDelta(3.6, 1e-9));
}
It(Test_With_Fluent_Expression_And_Message)
{
Assert::That(3.5, Is().EqualToWithDelta(3.6, 1e-9), WithMessage("Invalid result for input: 3.5"));
}
It(CompatTest_With_StringLiteral_Fluent_Expression_Without_Message)
{
Assert::That("a", Is().StartingWith("b"));
}
It(Test_With_StringLiteral_Fluent_Expression_And_Message)
{
Assert::That("a", Is().StartingWith("b"), WithMessage("Invalid result for input: a"));
}
It(CompatTest_With_Expression_Without_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9));
}
It(Test_With_Expression_And_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), WithMessage("Invalid result for input: 3.5"));
}
It(CompatTest_With_StringLiteral_Expression_Without_Message)
{
Assert::That("a", StartsWith("b"));
}
It(Test_With_StringLiteral_Expression_And_Message)
{
Assert::That("a", StartsWith("b"), WithMessage("Invalid result for input: a"));
}
It(CompatTest_With_Boolean_Without_Message)
{
Assert::That(false);
}
//does not compile
//It(Test_With_Boolean_And_Message)
//{
// Assert::That(false, AssertionMessage("Wrong answer!"));
//}
};
Describe(MessageSuppliers) {
Describe(Test_StaticMessageSupplier) {
It(Test_With_Static_Message)
{
Assert::That(3.5, EqualsWithDelta(3.6, 1e-9), WithMessage("Invalid result for input: 3.5"));
}
};
Describe(Test_LazyEvaluationSupplier) {
It(Test_LazySupplier_Lambda) {
for (int input = 0; input < 100; ++input) {
//Potentially expensive way to build an assertion message.
//Postpone it until actually needed, do not calculate the message
//if assertion passes.
auto makemsg = [input]() -> std::string {
return std::string("Invalid answer for ") + std::string(input, '#');
};
Assert::That(input, IsLessThan(50), makemsg);
}
}
};
};
Describe(Booleans) {
It(With_ExpressionBuilder_No_Message) {
Assert::That(true, Is().False());
}
It(With_ExpressionBuilder_And_Message) {
Assert::That(true, Is().False(), WithMessage("Wrong answer!"));
}
It(With_Expression_No_Message) {
Assert::That(true, IsFalse());
}
It(With_Expression_And_Message) {
Assert::That(true, IsFalse(), WithMessage("Wrong answer!"));
}
It(Literal) {
Assert::That(false);
}
};
// [main]
#include <igloo/CodewarsTestListener.h>
int main(int, const char* []) {
NullTestResultsOutput output;
TestRunner runner(output);
CodewarsTestListener listener;
runner.AddListener(&listener);
runner.Run();
}Example output |
#include <fmt/core.h>
I bet C++20 (
We need to document this either way.
We shouldn't add code if there's no use case. Identity function exists because it's useful.
Yeah, authors can misuse whatever we provide. But I don't know how Even if we include it, that won't prevent them from writing their own, or causing maintenance work by writing awful code with them. It should be possible to add static assertions to restrict the type of the supplier somehow. I'd rather do that if we care. |
|
@error256 Can you review this and let us know your thoughts? |
Co-authored-by: kazk <kazk.dev@gmail.com>
| namespace snowhouse | ||
| { | ||
| auto WithMessage = [](const std::string& msg) { return [&]() { return msg; }; }; | ||
|
|
There was a problem hiding this comment.
I didn't realize how often we had to specify the empty message supplier.
Let's add
namespace detail {
auto EmptyMessage = []() { return std::string(); };
}and use that. That's more readable.
There was a problem hiding this comment.
Are you sure this won't violate the One Definition Rule? EmptyMessage will become an object, and it will be defined in every translation unit which would have assert.h included, leading to multiple definitions and linker errors. The same applies to WithMessage\ExtraMessage defined as a named symbol in line 27. I might be wrong, and my VS2019 seems to compile and link correctly when I add another translation unit which includes Igloo, but I am not 100% sure it's valid code.
Am I wrong?
There was a problem hiding this comment.
I made both EmptyMessage and ExtraMessage inline functions to avoid violation of ODR and problems with linking. They could probably be made inline "named lambdas", but my VS2019 seems to not like inline variables and fails to compile them. I hesitate to make them "regular, named lambdas" because I think they might violate ODR (I am not sure though, my C++ got really rusty :( ), but if you are sure they won't, I can do it.
There was a problem hiding this comment.
I don't know C++ much. It might be a violation. https://stackoverflow.com/a/62150779
I thought assert.h having a header guard helps, but not sure.
There was a problem hiding this comment.
Guards won't help because then it won't be declared in other translation units. I think inline variables should be good. What do you mean by "regular, named lambdas"? Usual functor classes can use inline member functions, so I don't think they'll have ODR issues.
Are lvalue instances of WithMessage/ExtraMessage supposed to exist? I haven't looked at the code thoroughly, but with using strings by reference it looks like there will be some issues if someone creates an lvalue WithMessage from an rvalue string.
There was a problem hiding this comment.
I wanted to use them, but my VS2019 complained. Either it uses C++14 compliant compiler, or this C++17 feature is not supported, or I did something wrong.
This is the first time I've seen inline variables, but it looks like this is exactly the case they exist for. What about static?
but since the thing is named, it's not a lambda anymore, right?
I guess a lambda is always a lambda. Its class name is always unique and unknown.
such variables defined in a header can (can they really?) violate ODR.
I think they can. If you repeat it in another translation unit, it's the second definition, otherwise there's no declaration. Normally, a definition should be in a separate translation unit, but it's a header-only library.
There was a problem hiding this comment.
Anyway, I think that EmptyMessage and ExtraMessage are perfectly fine as they are: inline, free functions. I'd say there's no need, and no benefit, in making them lambda-initialized variables, be it marked as inline or static or whatever, just for sake of turning functions into lambda expressions. Is there anything in particular better in
inline auto EmptyMessage = []() { return std::string(); };vs
inline std::string EmptyMessage()
{
return std::string();
}I personally think there's not.
There was a problem hiding this comment.
To be clear, I don't care if these are lambdas or functions, and I don't think there's a significant difference we'd care.
Originally suggested changing because I thought structs were unnecessary, especially since those were brought into scope. The lambda version compiled and worked as expected, so I thought that was good (spoiled by Rust).
Tested the latest version against all published C++ kata and had the same result (no error or new warning).
There was a problem hiding this comment.
Tested the latest version against all published C++ kata and had the same result (no error or new warning).
Great! When (and if) we get it deployed, I could test it on some kata from C++ list when fixing warnings.
There was a problem hiding this comment.
I would create some docs for cpp too similar to what we have for C or JS: some kind of reference, and authoring tutorial.
- fixed indentation - argument names changed to snake_case - removed commented code - renamed `WithMessage` to `ExtraMessage` - reintroduced `EmptyMessage`
kazk
left a comment
kazk
left a comment
There was a problem hiding this comment.
Looks good to me, but I want someone who knows C++ more to review. I'll try testing against existing kata to make sure this doesn't break them.
kazk
changed the title
3 participants
Another iteration on idea from #1 (comment) .
Proposed API:
MessageSupplierTypetype parameter ofAssert::Thatwould allow for different implementations of message suppliers.MessageStringSupplieris provided by default and can be constructed withExtraMessageconvenience function, but it would be possible to create other suppliers too, for example utilising string streams or{fmt}.