Make it easier to test and/or develop on Windows about better-enums HOT 19 OPEN

I've assigned myself to this; it's a good opportunity for me to familiarize myself with building the project but also look into addressing the issue. At $DAYJOB, I use the Catch test library for unit tests and it works well. I use CMake + CTest to execute the tests on the CI build server.

from better-enums.

Hmm not sure what you mean. The one I'm talking about is one I wrote from scratch in the cmake-tests branch (it started on my fork, but then I pushed it up to your repo to test the travis builds). I do remember you linking to another CMakeLists.txt in a fork some days ago, but I did not end up using it since I wanted to do much more.

I had a few big goals in mind:

• Get rid of direct Make/Python dependencies (if reasonable to do so, I'm still looking into this one)
• CMake INSTALL targets for better-enums for distribution purposes
• Rewrite tests using Catch test library
• Integration of CTest for running unit tests

BTW side note: CPack allows you to build an installer for better-enums based on the install targets I've setup. Never used CPack before but it's a future benefit, if you ever wanted to distribute in this manner: https://cmake.org/Wiki/CMake:Packaging_With_CPack#Using_CPack_with_CMake

from better-enums.

That all sounds good. Some of the tests are actually the tutorials and the advanced demos, and there are some Python scripts that convert them from Markdown to C++. When I first published Better Enums, I was new to maintenance and thought it was a good idea that some of the docs were also tests, but I wouldn't have done that now.

So, if those are an obstacle in your work, just exclude them. We can add equivalent tests later, or, for now, we can generate those C++ test files once and commit them – whichever you find easiest/best.

In general, if you go the Catch route rather than installing Cygwin or Win32 ports, feel free to create equivalent tests as necessary, if porting some particular tests is a pain.

from better-enums.

Also, FYI, @mitsutaka-takeda added a CMakeLists.txt file for the library itself in this commit in a fork, and has graciously given permission to cherry pick it (thanks!). I didn't give it a proper evaluation, but if it helps, we can take it (or use it for reference).

from better-enums.

Well since you are (currently) a header-only library, there is very little benefit to having a CMake script outside of building unit tests. You might provide one in releases of your library for find_package() support, but even that isn't too important since you only have 1 header to include.

from better-enums.

@aantron Could you explain what the test/expected directory is for? It seems like expected output for some tests, however none of the test code so far I've seen actually spits out data that is compared like this. I see the relevant code in test/Makefile:

run-tests :
    $(BIN)/cxxtest
    @for FILE in $(BIN)/example-*$(SUFFIX) ; \
    do \
        EXAMPLE=$$(basename $$FILE | sed s/\.exe$$// | sed s/^example-//) ; \
        $$FILE | sed 's/\r$$//' | cmp - expect/$$EXAMPLE ; \
        RESULT=$$? ; \
        if [ $$RESULT -ne 0 ] ; \
        then \
            echo \'$$FILE\' produced bad output ; \
            exit $$RESULT ; \
        fi ; \
    done
    @echo Example program output matches expected output

So far I have all the unit test code (under test/cxxtest) compiling, running, and passing through CMake under the Catch test library. I want to make sure I'm not missing other things.

from better-enums.

Oh I think I just found the answer. Under example directory you have what appear to be more tests. Would you like me to convert these to actual unit tests? We can compare the output in code instead of depending on external files.

I get that these are supposed to be introductory examples, however it's not uncommon to use unit tests as examples to help people learn a library. You could have each test case self-contained so that the expected output is visible from the code being tested, allowing users to set breakpoints and step through to figure out what is going on as needed.

Let me know.

from better-enums.

Wow I just came back and I'm seeing all the spam done here due to the rebased commits I pushed up. Sorry for that! I was doing lots of pushes to get the travis build working. I hadn't used it before, and it wasn't building branches on my fork, so I wasn't sure what else to do.

Anyhow, I have the travis build mostly working on my branch (cmake-tests). My work is still incomplete, as I've removed all build configurations from the matrix except for clang 3.5. I want to get that one working before I add others.

There is operator ambiguity in the following test case:

TEST_CASE("test alias", MAIN_TAG)
{
    CHECK(Compression::Default == Compression::Huffman);
}

Located in test/test_enum.cpp. See this build: https://travis-ci.org/aantron/better-enums/builds/143782082. I would quote the error here, but it's rather lengthy and you can see it via the link provided here.

I wanted to get some help looking at this problem before I continue further. I'm not even sure if this test case passed before, or if it was even executed. It could be some mistake I made at the toolchain level as well.

Let me know what you think. Thanks!!

from better-enums.

Thanks for your work so far!

Well since you are (currently) a header-only library, there is very little benefit to having a CMake script outside of building unit tests.

Right, precisely why I didn't include CMakeLists.txt for the whole project, originally. But, the linked commit mentions integrating Better Enums with other projects. I've never used CMake for that, so I don't know what's best to do here. Perhaps @mitsutaka-takeda can give the rationale in some more detail, now or at a later date.

Oh I think I just found the answer. Under example directory you have what appear to be more tests. Would you like me to convert these to actual unit tests? We can compare the output in code instead of depending on external file

Right. As for converting them to tests, use your discretion. Yes, it's common to demonstrate a library using unit tests, however these examples are also converted to HTML for display (e.g.) here. As I mentioned, whether having these dual-purpose files is a good idea is very much up for debate, so use your discretion. I was planning to rewrite the docs anyway at some point. They seem a bit over-the-top. It was my first open source release :p I'm leaning towards just removing the examples from testing and covering everything with new tests, as you propose. Don't delete the Markdown files for now, they are still used to generate the docs.

Wow I just came back and I'm seeing all the spam done here due to the rebased commits I pushed up.

Don't worry about the spam :) And you can use the main repo (as you have been). In such cases, I typically work on some commit for a while, then do a final --amend to insert the issue/PR reference before pushing to master/rebasing the PR for final merge by maintainer. If I need to rework afterwards, I remove the issue reference again, and then replace it, etc. Another way to deal with this, if you are willing to create a PR, is to insert the issue reference into the PR only. Then you can rebase as much as you want in the PR. You may already know that – just being thorough :)

There is operator ambiguity in the following test case:

Yes, this test was running – just confirmed by messing it up, and it fails in the original CxxTest code.

The ambiguity is caused by enumerands of Compression being of unscoped enum type. Catch applies operator <<, but Compression::Huffman is convertible to integral types and to Compression. This is another case of operator ambiguity to resolve, and contributed to me deciding to just uniformly require operator + on bare constants :/ As a user, you would do << +Compression::Huffman. But, obviously, we can't force Catch to do that. I guess this is another hint that conversions need to be better addressed.

It seems like you will have to omit such tests for now, keep the Catch code in a branch, and we will have to address this conversion issue before the Catch code can be completed. There may be a better way, that's just off the top of my head.

from better-enums.

So would it be fair to say that we leave the examples out of the unit tests? You stated you had planned to pull them out anyway.

The real question is, do you feel that the unit tests (excluding the examples) provide sufficient test coverage today? If the answer is no, would converting the examples to unit tests meet your expectation of sufficient test coverage? If the answer is yes, I'd be happy to look into it. If the answer is still no, the problem seems to be much deeper and for now I'd leave the examples out of the test scenarios.

from better-enums.

So would it be fair to say that we leave the examples out of the unit tests? You stated you had planned to pull them out anyway.

In their current form, yes.

The real question is, do you feel that the unit tests (excluding the examples) provide sufficient test coverage today?

No. For example, you can see that there isn't a single switch statements in the two "dedicated" test files.

would converting the examples to unit tests meet your expectation of sufficient test coverage?

Yes, but there will be some redundancy, you'd just have to glance through the files and see what isn't already done in the unit tests (e.g. switch). Only the tutorial examples (numbers less than 100) are needed for proper coverage.

from better-enums.

Also, the root CMakeLists.txt defines better-enums as an interface library (interface libraries are designed for header-only libraries). It also generates a config package which can be installed for distribution.

The way your library is currently setup to be installed is via the following structure:

<CMAKE_INSTALL_PREFIX>/include/better-enums-<VERSION>
<CMAKE_INSTALL_PREFIX>/lib/cmake/better-enums-<VERSION>

The lib/cmake directory contains the config package script, version script, and import targets script. Downstream CMake projects can then import your library via find_package():

cmake_minimum_required(VERSION 3.1)
project(downstream)

find_package(better-enums 0.11.2 REQUIRED CONFIG)

add_executable(downstream main.cpp)
target_link_libraries(downstream better-enums)

Above, when find_package() is invoked, it searches a number of places for the install dir of better-enums package. From there, it loads the various cmake configuration files to confirm the version (optional to specify; since this allows multiple versions of better-enums to be installed on a system), as well as import any INTERFACE targets.

The INTERFACE targets are important. They allow downstream targets to specify better-enums as an actual link library, which (transitively) imports public headers, include directories, preprocessor definitions, and link libraries.

This makes it very easy for other CMake projects to utilize your library without requiring better-enums as a submodule in their repository. It can simply be another library installed via apt-get (linux) or via installer EXE on Windows (where it might be in C:\Program Files or similar location).

I'd like to document this all for you somewhere, but I just haven't gotten to that yet. Hopefully that helps fill you in.

from better-enums.

Thanks.

Just to be clear, this root CMakeLists.txt isn't in Better Enums now, it's in a fork. Your explanation sounds like it would be good to cherry-pick it over here. Is that correct?

As for documentation, I'm guessing that CMake users will already know what to do. Otherwise, we can add a sentence to the README and/or website at a later date :)

from better-enums.

Ah, fair enough. I was talking about the linked one.

I agree with all the goals, at least for unit testing. We may still need to keep Python around for docs for now, but that's a separate issue at this point.

from better-enums.

Sure, I'm not on a witch hunt or anything. That's why I left the "if reasonable to do so" there, because quite honestly I'm not sure what is needed for what right now. I want to cover the cases where either make or python or both are needed for building tests themselves. If it's needed for documentation I will obviously ensure that it stays and continues to function. We could even have CMake execute the python steps for us as well, unless you plan to make the travis script do that.

from better-enums.

Sure, I'm not on a witch hunt or anything.

No worries, I understood you :)

It's likely that there won't be any Python steps at all for the docs, if/when I rewrite them. The rewrite will change the generation process.

from better-enums.

@aantron Could you take a look at these compiler errors on GCC 4.6 and let me know what you think?
https://travis-ci.org/aantron/better-enums/jobs/145157666

Looks like there is a bug in GCC on this:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54086

Fixed in 4.7. 4.7 does work, and I think 4.5 works because CONSTEXPR is not supported. Thoughts?

from better-enums.

Yep, this is (was at the time) a well-known GCC bug in their constexpr support. I remember dealing with it back when GCC 4.6 was current :p

In the master Better Enums CMakeLists.txt, I hardcoded 4.6 as not supporting constexpr for this reason:

So, it should be fine to do the same on your end, if convenient.

from better-enums.

Ok so somewhere along the line I probably broke that specific set of code in the CMake script. At first I was thinking if we need to remove 4.6 from the matrix, but that's not the case.

Thanks.

from better-enums.