Unit Tests

#1 I guess, for a start, we can begin to test all the exposed TS console functions in regular unit-test fashion. While I don't expect that getWord, for example, will break because of any community contributions, it'll help us set up a good workflow and test framework. And it might help us document the script API a bit more strongly - for example, I found some inconsistency in handling of records and fields which I thought should be cleared up. Writing unit tests forces us to think about how these functions should behave and confront the reality of how they do behave.

Since we're on the subject, I'll point out tstests, where I'm trying to start writing unit tests for TorqueScript in TorqueScript. The tests are in the test/ folder. Here's an example:

// Declare a new test suite for MyObject.
test(MyObject);

// Set up before each test.
function MyObjectTests::before() {
   new ScriptObject(MyObject) {
      property = "foo";
   };
}

// And pull everything down afterwards!
function MyObjectTests::after() {
   MyObject.delete();
}

function MyObjectShould::do_something_silly() {
   expect(MyObject.property).toBeAVector(3);
}

And example output for a suite with a deliberate failure:

#2 Well ideally when you add or modify code you should write a unit test to verify each addition/change made and set it up to run in a "test build" or some such.

In practice I never do this - it's a pain. Bad programmer, no cookie for me....

#3 Thanks for doing this, should be helpful with my TorqueScript Improvements to ensure it don't blow anything up which are in slow progress but coming sometime soonish.

#4 @Richard
Oh, definitely. Of course, 99% of programmers won't ever do that. If we weren't lazy, we wouldn't be programming :P

We're never going to write unit tests that cover everything, but we CAN build a suit of stuff that is testable.
Especially with all the improvements/expansion of the rendering and platform stuff, I feel that should should get a pretty heavy unit test focus to make it easier on the guys working on it if their changes break everything or not.

#5 Hi Jeff--

Unit tests are great in some cases, but it's easy to go overboard, and you really want a continuous build system to make them work. Unit tests tend to follow the 90/10 rule; really thorough unit testing can catch the 90% of bugs that require about 10% of the time to track down and fix, but they're completely unable to find the 10% of really hard bugs that take 90% of the time to track down and fix.

I think the obvious place to start with unit tests is with /core/... and /math/... When I refactored the Torque math libraries, I wrote ~2500 lines of unit tests to ensure consistency, and it was a huge help in tracking down issues.

I'm surprised you mention rendering, though, because it definitely falls into the camp of things that are nearly impossible to track down with unit tests.

#6 Good points.

I haven't every really used unit tests myself, simply because I personally implement things (more or less) one at a time. So if it breaks, I have an idea where.

Unit tests in terms of pushing changes through a basic 'did this break in weird places' test seems logical, but you're completely right that a lot of it either needs specific written tests(such as a unit test written by the guy making the change) or it's probably not going to be useful at all.

I didn't mean to imply with this thread we need a 'TEST ALL THE THINGS!' approach, but my personal unfamiliarity with just how extensive the tests need to be means I need suggestions from other people on what kind of tests need to be there.

Also, when I mentioned rendering, I was thinking in terms of 'Can the engine allocate vertex buffers and use them with standard GFX calls?' kind of thing.

You get major overhauls to GFX and functional parity between the platforms underneath is pretty important.

Weither or not it's actually feasible or warranted? No idea. 'S why I was asking for input :)

#7 Okay, I reckon we should make this happen sooner rather than later. As in, for 3.6 if at all possible. And by 'this' I mean having a solid unit test framework in place so it's easy enough for anyone who wants to to add unit tests for parts of the code they're involved in.

We (the steering committee) have been given access to GarageGames' TeamCity CI server, which is currently set up to run nightly builds from the development branch. As you can see here. But to make it really helpful we should also have a step that runs the unit tests as well.

Adding tests for maths and stuff is a good idea. I mean, we can be pretty sure most of it works, because it's been in use for years, but, for example, someone realised that quaternion conversion is broken, and I found some methods in Box3F that are misnamed or just not actually implemented. It'd be great for us to incrementally add tests for stuff like this, both to ensure our API actually does what it advertises, and to prevent regressions if we do decide to, for example, refactor maths functions.

Also, if we look at, for example, adding multithreading to core classes like the bin system or the StringTable, we'd want to have test suites in place to prop up their API promises.

Following discussion on Jeff's pull request, I reckon our first step should be to pull the unit test framework out into a separate module, and have a new directory for tests. Or maybe subdirectories in relevant places (i.e. math/tests, console/tests). As long as we can stop them from being added to regular builds.

EDIT: A good mantra would be that if you submit a bug fix, also submit a unit test for it.

EDIT: Actually, let's please definitely move all the unit tests to component-specific folders. That way test files can be introduced by plugins cleanly, instead of having to copy files to the unit test folder. This may require some Project Manager shenanigans. Hmm. Or maybe just a unit tests #define.

#8

Quote:-Tim Condon:
....and you really want a continuous build system to make them work.

Indeed - Someone on the committee should talk to Scott Burns about using the system already in place at GG. Actually, when you commit back to the master branch it might already be rebuilding - I didn't personally update the TeamCity T3D project to point to Github but someone probably did. If this doesn't feel accessible enough then someone else could set TeamCity up and use it - it's pretty easy to use and you can use it for up to 20 build configurations (with three agents, so Windows, Linux, Mac builds could all fire off automatically on check-in) for free.

I don't want to sound like I'm pushing this product, but I actually use it at work and it's pretty slick. It's also what GG already has available.

#9

Quote:Indeed - Someone on the committee should talk to Scott Burns about using the system already in place at GG.

Quote:We (the steering committee) have been given access to GarageGames' TeamCity CI server, which is currently set up to run nightly builds from the development branch.

;)

#10 LOL - again, I should read the whole thing....

Still, nightly builds aren't exactly "continuous" - though close enough I suppose.

#11 A test suite of materials would also be a big help. The possible material feature combinations are nearly infinite, but a good representative sample could be achieved. A level would also need created that instanced each material under a sun, point and spot lights. With all of the gfx layer refactoring going on, this would be a good benchmark to verify that the majority of procedural shaders will generate, compile and execute. That way the committee could be spared spending time evaluating changes if they don't at-least pass a standard suite test. It seems most testing is done with the full template and default character which barely use any of the advanced material features and none of the advanced lighting features.

#12 Michael - when we talk about unit tests, we mean tests that can be run automatically without any human supervision. Usually they verify a single function or class. Though you're right, it'd be very helpful for us to have some way of verifying higher-level behavior. That's commonly referred to as integration testing.

Richard, yeah, it's not real CI, but it's not too bad. I've been using Travis CI for a smaller JavaScript project and it's great to have Travis run the test suite automatically on every pull request...

#13 Scott has mentioned that we can configure TC to test pull-requests, so I'll look at that sometime soon. I'd also like to draw attention to this issue which is causing some problems with our automated unit tests.

#14 Maybe we should use Google Test like T2D?

#15 I have a working google test integration with the engine, ready to be pull--requested. Question: for 3.6, should we try to port all existing unit tests to gtest and remove the current test framework? Or should we let both frameworks coexist in this release and port them slowly?

#16 I'd go for a stepwise merge mself. lets folks get a practical how-to example when it comes to porting any of their own they may have cooked up.

#17 Okay so as part of my experimentation with documentation, I did a write up of the current unit test framework, which has given me a good idea of some things to do/improve.

The first thing I found was that the exiting unit test system really isn't bad. It's kind of fine actually. Google test has slightly less boilerplate for simple tests but it's not excessive. Google test does have more assertions built in, and allows you to use stream syntax to add error messages, which is really nice. I'm going to have to put a little work into allowing a subset of tests to be run, but that's not critical anyway.

I'm thinking that for our CI pull request checking, lots of the current unit tests won't be the most helpful. For example, the 'default allocation' test I went over in my post actually seems designed not to pass. Huh. So I doubt I'll be porting that test :P. Also, lots of tests seem to be 'stress' or performance tests, which we don't necessarily want to run on every PR, especially since they make the whole thing take ages.

Az, I hear you. I'd actually be surprised to learn that anyone has been writing unit tests for themselves to be totally honest :P. But I'll at least leave the current framework in place, PR the new framework, then set about replacing all tests gradually.

EDIT: Something else I just discovered: the MemoryTester class is actually a stub :P. That was disappointing :P. But I discovered this handy plugin for Google Test that might help detect memory leaks in Visual C++...

EDIT: This is funny. The memory leak checker was showing a failure every 4 tests, so I deduced it might have something to do with the reporting. Lo and behold, I find this in Con::printf. Hmm. So I wonder if anything's to be done about that. I disabled the whole block, but it leads to a blank console window. I'm going to need to dig a little deeper.

EDIT: Basic googletest in #706.

#18 I guess I'll keep talking to myself about this ;P. Here's an example of how the new unit tests will work. They look so nice because I refactored them as well as porting to googletest. No more 20 line repetitions to push elements into vectors :P. Also note that the memory leak check that was previously intended to happen in testVector now actually happens thanks to the memory checker plugin I found for googletest.

#19 An update on my progress. I discovered that the memory leak checker is problematic because both the console and the profiler introduce deliberate memory leaks as part of their operation :P. So, that sucks. You can fix the console memory leak by changing a global variable ($Con::LogBufferEnabled = false), and I've also added a new flag ($Testing::CheckMemoryLeaks) that will enable the memory leak tester (it's disabled by default), so if you want to bend over backwards you can.

At the moment the only solution to the profiler leaking is to disable it entirely, which I suspect is a matter of enabling TORQUE_SHIPPING. I'm experimenting with that right now. I'd prefer that the profiler simply allocated no memory when it's not running - or at least, freed its allocated memory entirely in a single profiler run - so we could have it enabled while checking for leaks. No idea how feasible this is, though.

Should there be a separate define, like TORQUE_DISABLE_PROFILER, which overrules the profiler enable flag? Or should we just stop TORQUE_ENABLE_PROFILER from being automatically enabled in non-shipping builds and make it a per-project flag to be set by the generator (default to on)? Of course there's no easy way to then remove that definition, unless it's written in one of the per-project .conf files, where users can delete the line. Hmm.

EDIT: TORQUE_SHIPPING only works in a release build, or assumptions break and stuff gets weird. I don't particularly like the idea of only being able to run unit tests in a debug build, and this all precludes the possibility of actually writing unit tests for the profiler itself. Hmm.

#20 Why does the log buffer leak? Could it be replaced with a vector of log entries? Could we set a maximum size and cause it to roll - it dumps to console.log so it really doesn't need to be kept in memory in my opinion; it could be a window on the last thousand lines or something.

Ultimately, I think the memory leaks should simply not happen. I'm having a hard time coming up with a scenario where having a memory leak is a design goal, or even a constraint. Thanks for looking into this Daniel.