Testing utility library

[delcypher]

I've been taking a look at some of the tests and I think we are in need of some sort of utility library to assert properties in tests.

There seems to be mix of different styles. Here are some examples

    if (result_f != 50) {
        printf("Arg max of f is %d, but should have been 50\n", result_f);
        return -1;
    }

    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 5; j++) {
            float in = input(i, j);
            float expected = in * scale(0, 0) * (negate(0, 0) ? -1.0f : 1.0f);
            assert(output(i, j) == expected);
        }
    }

    if (!target.has_feature(Target::OpenGL))  {
        fprintf(stderr,"ERROR: This test must be run with an OpenGL target, e.g. by setting HL_JIT_TARGET=host-opengl.\n");
        return 1;
    }

Having the tests like this present some problems

• The assert() tests don't do anything on Release build because asserts are disabled!
• return 1 isn't so great for debugging in gdb because the process exits normally. abort() works better for this use case because gdb catches SIGABRT.
• (minor) Inconsistent style confuses new developers (i.e. me) because they don't really know which to use.

So I think we need some sort of utility library to assert properties so that it is done consistently and without the issues I mentioned above. I noticed that Halide has a built in assertion mechanism (e.g. internal_error()) for doing this already is there a reason that is not being used for the tests?

Alternatively we could use something like GTest to assert properties.

[steven-johnson]

+1 to moving towards GTest.

[abadams]

I've been using return or exit(-1), but your point about gdb is good - I think abort would be better.

The likely reason we don't use internal_error is because they get undef'ed at the end of Halide.h to not pollute the user's namespace, so they're not available. We could make them available somehow, or there's _halide_user_assert. Those macros have the advantage of printing line numbers, unlike abort.

On Wed, Aug 5, 2015 at 8:10 PM, Steven Johnson notifications@github.com wrote:

+1 to moving towards GTest.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/898#issuecomment-128223520.

[abadams]

How about we rename _halide_user_assert() to just halide_assert(), and add a halide_error() to match, and then use those?

[abadams]

(and use abort in tests that don't use Halide.h - e.g. aot stuff)

[delcypher]

The likely reason we don't use internal_error is because they get undef'ed at the end of Halide.h to not pollute the user's namespace, so they're not available. We could make them available somehow, or there's _halide_user_assert. Those macros have the advantage of printing line numbers, unlike abort.

We could make them accessible by including Error.h directly right?

[delcypher]

How about we rename _halide_user_assert() to just halide_assert(), and add a halide_error() to match, and then use those? (and use abort in tests that don't use Halide.h - e.g. aot stuff)

I forgot about the AOT tests. I'd like the way properties are asserted to be consistent across all tests so I think it would be better to not depend on libHalide and its header files. If we go down this route we either need to write our own very simple utility library (it could just be a simple header file) or use GTest.

I've converted projects to using GTest before and in my experience you basically need your own copy in tree because it really needs to be compiled with the same flags you use for the rest of your build. Either that or you ask users to pull the latest GTest (e.g. as a submodule) but that is a bit of a pain. On the plus side by using GTest we get to use its nice assertion language and also have multiple tests in a single executable if we want.