Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests #362

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

pgruenbacher
Copy link
Contributor

@pgruenbacher pgruenbacher commented Jul 9, 2019

the MACRO bs_add_test is added. it could be cleaned up but it works for now. it's more up to marko to decide.

make test and ctest should now work when in the Build dir. enable_testing needs to be at root dir. octree

there's still an unhappy lack of testing verbosity but I guess that's a future implementation effort.

/usr/local/bin/ctest --force-new-ctest-process 
Test project /home/pgruenbacher/build/bsframework/bsfMaster/Build
    Start 1: UtilityTests
1/3 Test #1: UtilityTests .....................   Passed    0.52 sec
    Start 2: CoreTests
2/3 Test #2: CoreTests ........................   Passed    0.52 sec
    Start 3: RenderBeastTests
3/3 Test #3: RenderBeastTests .................   Passed    0.52 sec

@@ -0,0 +1,50 @@
//************************************ bs::framework - Copyright 2018 Marko Pintera **************************************//
//*********** Licensed under the MIT license. See LICENSE.md for full terms. This notice is not to be removed. ***********//
#include "Testing/BsConsoleTestOutput.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep these non-unit tests elsewhere. Generally these things might require custom assets and might be bigger than one file as well, so it makes sense to me to dedicate a space for them. Even though this is a simple one I feel it fits into that category.

Perhaps we can add a Tests folder to bsfExamples repository? Or a custom repository.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also applies to BsUtilityTest.cpp above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh i prefer tests to be close to the code of interest. otherwise they can too quickly go out of date.
unit-tests shouldn't be too dependent on assets. unit-tests != render tests != gameplay tests and can be separate. I'd prefer to be able to add a lot more tests to validate the serialization, threading, memory, perf. and math.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue with separate repos is that the most common example is a method signature changes, and then the author needs to remember to go to the separate repo, and update it there as well. Ideally the dev should just be compiling with build_tests=ON and so the tests are always compiling along with any signature changes.

Copy link
Contributor Author

@pgruenbacher pgruenbacher Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyone compiling the source code should be running the unit-tests on their platform as well to validate. And I don't find files ending with *_test.cpp to be confusing when intermixed with source code. Having FileSystem.cpp and FileSystem_test.cpp in the same folder is a lot more intuitive than in a separate repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Because you are starting up the application I assumed this was doing more than just a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah no, starting up the application was only necessary for starting some of the modules. some of the tests involved code that had to be initialized by the modules. At the very least Platform, MemStack, MessageHandler had to be startUp(). otherwise any tests using memstack will fail obviously.

@@ -192,6 +192,35 @@ MACRO(end_find_package FOLDER_NAME MAIN_LIB_NAME)
set(${FOLDER_NAME}_INCLUDE_DIRS ${${FOLDER_NAME}_INCLUDE_DIR})
ENDMACRO()

MACRO(bs_add_test)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this macro vs. using add_test? Also, I'm seeing references to Debug and Release - does it work on other configs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i updated with commit for explanation. Esentially I was getting "found .so library" errors until I updated the tests. plus it's just useful to have add_test wrapper for future in case there's other common properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know if it works for other configs. But it at least works for my needs at the moment.

@BearishSun
Copy link
Member

Thanks for making the changes, I made some notes but in general it looks good.

@@ -3,14 +3,48 @@
#include "Testing/BsConsoleTestOutput.h"
#include "Private/UnitTests/BsUtilityTestSuite.h"

#include "BsApplication.h"
Copy link
Member

@BearishSun BearishSun Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I organize the non-plugin code in layers. 'Utility' layer is the lowest one, and it shouldn't be referencing anything from the higher layers (this includes Application or CoreApplication). Are these tests failing without the Application started up? Ideally we can run them without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm. well we could just change it to BsCoreApplication, which is the core layer, and therefore compatible with all other layers of testing for most part. I'll make the change and see if that works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope nvm, CoreApplication can't be initialized by itself for running the utility tests. stuff like gProfiler still gets called

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants