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 #133

Merged
merged 81 commits into from Feb 25, 2017

Conversation

Projects
None yet
3 participants
@DrMcCoy
Member

DrMcCoy commented Dec 21, 2016

This PR adds general unit tests (using Google Test) to xoreos.

It's not 100% coverage, far from it. There's currently 1034 indidual tests in 70 file (with ~ one class per file), covering most of Common, many of the Aurora classes and 3 files for image decoders.

Included in the changeset is also some CMake changes. @berenm, if you could double-check that what I did there makes sense, that would be great. Thanks.

I'm also enabling the unit tests in Travis CI and AppVeyor. The former seems to work, but for some reason, AppVeyor throws errors there. "Exception: Other", whatever that is supposed to mean. I don't suppose you might know what the issues is there, or how I can get more usual debug info, @berenm? Because that is just weird :/.

Going forward, more unit tests would be great. For example:

  • PE EXE file in common/ and PE file archive in aurora/.
  • Image decoders. Possible, but annoying to write.
  • Nintendo DS formats.
  • Aurora::ResourceManager? Would need functions to manipulate some structures more directly.
  • FFT/DCT/MDCT/RDFT? Would be fiddly.
  • Sound decoders, probably rather difficult to test. Video decoders doubly so.
  • Fonts and models would be neat, but also difficult. And currently, the loaders directly use OpenGL objects...
  • Engine-/game-level tests? World object structures? Script functions?

Likewise, new or changed functions in classes already covered by the tests should now be getting unit tests as well.

Comments, criticism, etc. welcome, of course. :)

#include "src/aurora/2dafile.h"
#include "src/aurora/gdafile.h"
static const byte k2DAASCII[] =

This comment has been minimized.

@clone2727

clone2727 Dec 21, 2016

Contributor

Should be char?

Yes, I know it ends up in a MemoryReadStream, but it seems odd to store text as byte here.

GTEST_TEST(BIFFile10, getNameHashAlgo) {
Common::MemoryReadStream *stream =
new Common::MemoryReadStream(kBIF10File, sizeof(kBIF10File));

This comment has been minimized.

@clone2727

clone2727 Dec 21, 2016

Contributor

Having to type the variable twice always bugs me, especially when it's easy to have a copy/paste error. Maybe it would be better to have something like this as a function in MemoryReadStream:

template <size_t N>
static MemoryReadStream *CreateFromArray(const byte (&array)[N]) {
    return new MemoryReadStream(array, N);
}
"Which yet survive, stamped on these lifeless things,\n"
"The hand that mocked them and the heart that fed:\n"
"And on the pedestal these words appear:\n"
"\'My name is Ozymandias, king of kings:\n"

This comment has been minimized.

@clone2727

clone2727 Dec 21, 2016

Contributor

You don't need to escape a single quote from within double quotes.

};
GTEST_TEST(ERFFile11NWN, getNameHashAlgo) {
const std::vector<byte> password(kERF11NWNPassword, kERF11NWNPassword + sizeof(kERF11NWNPassword));

This comment has been minimized.

@clone2727

clone2727 Dec 21, 2016

Contributor

Obviously can't modify std::vector, but a similar templated function as mentioned for MemoryReadStream might be nice to have.

@DrMcCoy

This comment has been minimized.

Member

DrMcCoy commented Dec 22, 2016

(AppVeyor:)
Okay, after a lot of fiddling (grrrr), it seems that many unit tests fail with error code -1073741515, which is 0xC0000135, which is STATUS_DLL_NOT_FOUND. Of course, it doesn't say which DLL...

The ones that do work are:

  • tests_common_test_scopedptr
  • tests_common_test_disposableptr
  • tests_common_test_ptrlist
  • tests_common_test_ptrvector
  • tests_common_test_ptrmap
  • tests_common_test_binsearch
  • tests_common_test_maths
  • tests_common_test_vector3
  • tests_common_test_matrix4x4
  • tests_common_test_boundingbox

All other unit tests fail. The ones that work are all small, contained classes/templates and might not even need any DLL at all...

@DrMcCoy

This comment has been minimized.

Member

DrMcCoy commented Dec 22, 2016

Okay, the tests all build and run in AppVeyor now. The ones that read/write files still fail, though, because the paths don't exist.

endwhile()
string(REPLACE ";" " " FLAGS_MISC "${FLAGS_MISC}")
set_target_properties(${TARGET} PROPERTIES APPEND_STRING PROPERTY COMPILE_FLAGS "${FLAGS_MISC}")

This comment has been minimized.

@berenm

berenm Dec 23, 2016

Contributor

Does this work ? I didn't know about this APPEND_STRING keyword, and it is only documented for the set_property function, same question for the three other places it is used.

This comment has been minimized.

@DrMcCoy

DrMcCoy Dec 23, 2016

Member

Yeah, interestingly enough it does work. Or at least it does for me, when I tested it.

I found it recommended in a StackOverflow question here: http://stackoverflow.com/a/19161129 , that's how I got that idea. :P

Is there a difference between set_target_properties(...) and set_property(TARGET ...)? Should I rather use one over the other?

@berenm

This comment has been minimized.

Contributor

berenm commented Dec 23, 2016

Sorry for the late reply, I see that you managed to figure out why the tests weren't running on Appveyor, that's nice. The CMake part looks good to me, I'm just unsure about the keyword usage I mentioned above.

@DrMcCoy

This comment has been minimized.

Member

DrMcCoy commented Dec 23, 2016

Neat, thanks for checking! :)

Yeah, got the AppVeyor stuff figured out now (although copying the DLLs is a bit hacky). Took a lot of just trying out things, because I don't know jack about Windows anymore.

Because I have no idea how to get Windows to tell me which DLL is missing, I created an "artifact" in AppVeyor consisting of a test binary, downloaded it and looked into it with objdump on my machine. Turns out iconv.dll is needed for some reason (because of the Boost libraries, maybe?), so I added that to the downloads. And also copied all the DLLs needed to the bin directory.

I also now changed the tests that write into files to use the tmp directory (using Boost to query that directory) and fixed two unrelated issues (one in a unit test, one in common/). Now the unit tests all run and pass in AppVeyor as well. :)

* Note: this isn't really a valid, standard NDS file. It only
* fills the header fields we actually care about. If the NDSFile
* class is extended to care about the other fields as well, this
* file data has to be changed as well. */

This comment has been minimized.

@clone2727

clone2727 Jan 22, 2017

Contributor

Did you manually generate this? I think the test would be better if generated from some tool.

#define SKIP_RETURN_CODE 77
#endif
static const int kSkipTest = SKIP_RETURN_CODE;

This comment has been minimized.

@clone2727

clone2727 Jan 22, 2017

Contributor

Why bother with the kSkipTest constant at this point when the SKIP_RETURN_CODE macro already exists?

Shouldn't this logic be in a common place anyway?

EXPECT_STREQ(Common::FilePath::relativize("/other" , "/path/to/file.ext").c_str(), "");
/* Common::FilePath::relativize() only works with normalized paths...
* These here are common failure modes.

This comment has been minimized.

@clone2727

clone2727 Jan 22, 2017

Contributor

I don't like the behavior of relativize only working with normalized paths -- can't it just do the normalization itself?

(Also, makeRelative would be a nicer name, IMO)

* fractions of 2^fBits. That's pretty different from IEEE floats, so their
* representation of non-trivial fractions diverge quite a bit. Which means
* checking that the result is within a few double ULP doesn't make much sense.
*/

This comment has been minimized.

@clone2727

clone2727 Jan 22, 2017

Contributor

Wouldn't it make more sense not to treat the fixed point numbers as floating point?

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 25, 2017

Member

(I already talked with @clone2727 about this, so this is just for the record: nope, they have to be treated as floating point numbers, because the numbers are thrown into Matrix4x4 and OpenGL.)

DrMcCoy added some commits Oct 25, 2016

@DrMcCoy DrMcCoy merged commit 1a14ac9 into master Feb 25, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@DrMcCoy DrMcCoy deleted the test branch May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment