pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
353 stars 117 forks source link

Gtests01 #303

Closed ingo-h closed 3 years ago

ingo-h commented 3 years ago

Make gtests running with new user interface

Vollstrecker commented 3 years ago

Short summary: Looks OK.

Longer version:

Oh, and I still wonder why they don't appear in the tests here.

ingo-h commented 3 years ago
  • I still don't get the sense of compile.sh, they are linked to the lib, and in the process of creating that they are created, too. Sure, for a quick code-change this could help, But just make is still shorter.

It is three times faster. I manly use it for the tools and have moved it to that directory.

  • For what is this test_template meant, that does nothing and isn't even compiled?

It is a template for new tests and it is a quick check if googletest works. It is a test of the testsystem. Developer just starting with gtests can look at it as entry point. No problem to have it compile and run on every sanity check.

  • I still prefer to mark the failing test and run them anyway.

Already done yesterday but was removed from gtest/CMakeLists.txt. Will put it back soon after troubleshooting for tests with mocked functions. That are the only tests that fail but not on all tests. Detailed documentation of disabled tests is done on every test output.

Oh, and I still wonder why they don't appear in the tests here.

Don't know who has them removed.

Vollstrecker commented 3 years ago

Don't know who has them removed.

We have added meanwhile another check. At least in the file I see here, the check executed is not yours.

mrjimenez commented 3 years ago
  • I still don't get the sense of compile.sh, they are linked to the lib, and in the process of creating that they are created, too. Sure, for a quick code-change this could help, But just make is still shorter.

It is three times faster. I manly use it for the tools and have moved it to that directory.

It is ok to have tools that makes developer's lives easier. Add some comments to compile.sh explaining that.

  • For what is this test_template meant, that does nothing and isn't even compiled?

It is a template for new tests and it is a quick check if googletest works. It is a test of the testsystem. Developer just starting with gtests can look at it as entry point. No problem to have it compile and run on every sanity check.

Well, then make it be compiled. I am not against it, but please do clearly state the objective in the comments.

  • I still prefer to mark the failing test and run them anyway.

Already done yesterday but was removed from gtest/CMakeLists.txt. Will put it back soon after troubleshooting for tests with mocked functions. That are the only tests that fail but not on all tests. Detailed documentation of disabled tests is done on every test output.

I am having some difficulty in understanding what exactly you are talking about. There are two kinds of test:

  1. Those that have already succeeded and now are part of sanity checking.
  2. Those that have never succeeded and are part of the development of new features or for fixing old wrong functionality.

Tests in the first category are the ones that must be run on Github actions. Tests in the second category must have a way to be run outside github actions.

Oh, and I still wonder why they don't appear in the tests here.

Don't know who has them removed.

Also don't understand what you are saying. The commits are all in this page.

Oh, another thing. I am getting this message: "This branch cannot be rebased due to conflicts". The conflicts are in the files CMakeLists.txt. They all leave the files unusable. E.g.:

<<<<<<< HEAD
    find_package (GTest CONFIG)

    if (GTest_FOUND)
        add_subdirectory (gtest)
    elseif (DOWNLOAD_AND_BUILD_DEPS AND NOT GTest_FOUND)
        CmDaB_install (googletest)
        add_subdirectory (gtest)
=======
#   if (NOT WIN32)
        find_package (GTest CONFIG)

        if (GTest_FOUND)
            add_subdirectory (gtest)
        elseif (DOWNLOAD_AND_BUILD_DEPS AND NOT GTest_FOUND)
            CmDaB_install (googletest)
            add_subdirectory (gtest)
        endif()
>>>>>>> 0ec135f (Include gtest/)

Either one of you must do the rebase by hand, or you must tell me what to do, since I am not the best person to deal with this stuff. I would prefer the former, but I can do that if you think it is too clumsy. I understand, it is clumsy and it took me some time to get used to it. The command is:

git rebase master ingo-h-gtests01

From then on, just follow git's instructions. If you need help, please call me.

mrjimenez commented 3 years ago

This is the second conflict (gtest/CMakeLists.txt):

<<<<<<< HEAD
=======
addGTest (test_upnpapi test_upnpapi.cpp
    ADDITIONAL_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/../upnp/src/threadutil/
)

>>>>>>> 0564913 (Make gtests running with new user interface)
addGTest(test_UpnpHttpHeaderList test_UpnpHttpHeaderList.cpp
    ADDITIONAL_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/../upnp/src/threadutil/
)
mrjimenez commented 3 years ago

Well, I did my best and this is what came out: #305 .

If there are changes to be made, please do on top of this branch so that we can merge it using Github tools.

Lets continue on the other thread.