build failure: empty pattern

[dabrahams]

Awesome work here!

Similar to #1 but I think this can be classified as a bug. If I write // build failure: at the end of a line, I expect the library to validate that there's an error on that line, no matter what its content. Every string contains the empty string.

My current workaround is to stick the source file name in there, which is not very satisfying.

[iboB]

First the DSL is // build error: and not // build failure:, but if you got it to work with the file name, then I assume that's just a mistake when writing the issue.

Yes. The DSL does indeed not match just // build error: as it expects at least one non-whitespace character after that. More on that later, but there's seems to be a misconception based on what I'm reading:

The lib does not know of "lines". It collects all instances of // build error: and searches for any of the strings in the build output. The position or order of // build error: instances in the code doesn't matter. Writing it next to the line which is supposed to produce the error is just a matter of preference (you might also prefer to write it at the very top of the file, for example).

Now, if you want to test that a build fails for any reason, you just need to call icm_add_build_failure_test where no source is tagged as PARSE. As in the example here:

https://github.com/iboB/icm/blob/master/demo/build_failure_testing/test/CMakeLists.txt#L17-L23

However that is a Bad Idea™ ... and here I wanted to link to the associated blog post, but I saw that you commented so you must have read it.

The whole point of the library is to test concrete build failures. Testing for any build failure will allow tests to pass where the build fails because of typos or completely unrelated errors to the one that you actually want to test.

[dabrahams]

Yes, it was a mistake filing the issue.

Thanks for clearing up the misconception, and yes, I have avoided ever not using PARSE.

I understand that parsing compiler error messages is not strictly something you can do portably, but in practice C++ compilers are pretty consistent, so I'd love to see an option to verify the line number. At worst you could simply verify that the line number appears in the error output somewhere. You might get some false negatives but in practice it would work.

[iboB]

I think it would be possible to test for a line by matching something like filename.cpp(line) perhaps by introducing a new DSL keyword. Say // compiler error here or something.

My initial thoughts:

It would be pretty hard to identify the line number from CMake. It would involve parsing line by line and... ew... counters (counters in CMake are terrible)

Though the risk of typos introducing unwanted errors is lower (a single line vs the whole file), it still exists. Using your concrete test:

ADOBE_PRECODNITION(true, "message", 1); // compiler error here

Oops! Did you see what I did? The code says "PRECO-D-N-ITION". This is still a compiler error on that line, though not the one you want to test for.

Because of these I'm still reluctant to add the feature. It seems to me that it's more dangerous than helpful.

Specifically in your case, why not say // build error: ADOBE_PRECONDITION in the test? Surely the macro expansion will be featured in the error message on all compilers ("expanded from here... yada yada")

I think it's much better than blindly testing for any error on a specific line

[dabrahams]

I would tend to use the feature with a string to match in the error on that line, so it would be less error prone than the current feature.

[iboB]

I can see how this can be useful, though some questions still remain. In conclusion, supporting empty patterns is not in the roadmap for now.

Closing this in favor of #3