Sl/google test

[xinlipn]

The project is to move tests from CTest to GoogleTest, which takes advantage of the same input data e.g. for testing on CPU and GPU, and saves the intermediate results for later comparison. This would avoid computing repetitively on the same data set and thus reduce testing time.

• Moved cache.cpp from /test to google test folder /test/gtest
• Replace main() in cache.cpp with google test Macro TEST()

[atamazov]

@xinlipn Is this PR ready for review? If not, then can you please convert this PR into draft?

Could you please also add some motivation to the PR description? This is important for sharing info with external collaborators who do not have access to AMD Jira. Thanks.

[xinlipn]

@xinlipn Is this PR ready for review? If not, then can you please convert this PR into draft?

Could you please also add some motivation to the PR description? This is important for sharing info with external collaborators who do not have access to AMD Jira. Thanks.

Hey Artem, Thanks for the feedback. I have updated the comments with more background.

[atamazov]

@xinlipn

GoogleTest, which takes advantage of the same input data e.g. for testing on CPU and GPU, and saves the intermediate results for later comparison.

Interesting feature. Can you please provide me with a link to the description of it?

[atamazov]

[Note] Please also note that CTest is able to test whatever the user wants, while IIRC GoogleTest is suitable for testing of C++ programs only. Therefore all the "custom" tests will have to be left under CTest.

[atamazov]

@pfultz2 Can you please glance at this PR? Thanks!

[atamazov]

@pfultz2 Thank you, Paul.

[pfultz2]

Just curious, how does gtest handle passing parameters from CLI to tests?

Its not feasible to create tests cases for every combination of convolution, so we can enumerate the ones that are important and then we should be able to pass flags to run the combinations that were not listed. That is how the current test driver works, but it does a cartesian product of all configurations so it creates a lot of test cases.

From looking at the docs in gtest, it looks like we will still need argument parsing as gtest doesn't take care of that for us whereas catch2 lets us easily add our own command-line arguments:

https://github.com/catchorg/Catch2/blob/devel/docs/own-main.md#adding-your-own-command-line-options

Whats the rational for choosing gtest over catch? gtest/catch will work well for our unit tests, but not so well for our acceptance tests. I am not sure either one can do what are current acceptance test drivers do though, but it looks like catch might get us closer there(we could at least take advantage of Clara).

which takes advantage of the same input data e.g. for testing on CPU and GPU, and saves the intermediate results for later comparison

The current test driver uses the same data for the CPU and GPU as well and it can save the CPU results too, but it is too large for all the test cases we run(which why it is not enabled by default). I did try to use bzip2 but it seems uncompressing was just about as slow(or even slower) for most cases as recomputing the CPU version(zstd might be a better choice for this).

There are few cases where the same config produces different input values because we are calling the test differently and using std::rand in a lot of places.

To improve our current test drivers we need:

• Enumerate the test config we want instead of cartesian product of all values
• Stop using std::rand to generate values(an xorshf96_generator like what we use in migraphx is quite fast).

But using gtest/catch wont help fix any of these issues expect maybe forcing a refactoring of the tests(which is definitely needed).

[atamazov]

@pfultz2

To improve our current test drivers we need:...

We can also use naive GPU implementations instead of CPU to compute reference data. This feature is implemented in the driver, but not in conv tests yet.

maybe forcing a refactoring of the tests(which is definitely needed).

😉

[xinlipn]

@xinlipn

GoogleTest, which takes advantage of the same input data e.g. for testing on CPU and GPU, and saves the intermediate results for later comparison.

Interesting feature. Can you please provide me with a link to the description of it?

Thanks everyone for reviewing and suggestions.
@atamazov, here's a link about GoogleTest FYI:
http://google.github.io/googletest/primer.html

[atamazov]

@xinlipn If the idea behind re-using the same input and output data is based on fixture classes, then it requires serious redesign on existing tests.

[xinlipn]

Thank you @atamazov and @pfultz2 for your time and input. Yes, TEST_F() offers the same data configurations for different tests.

I pushed some changes as below:

1. Macro EXPECT_EQ is replaced with EXPECT_TRUE per Paul's suggestion.

2. I also started refactoring activation.cpp for a simple "--float" test case, which may need further improvement to include the logic in test/CMakeLists.txt,

[pfultz2]

Macro EXPECT_EQ is replaced with EXPECT_TRUE per Paul's suggestion.

So doing EXPECT_TRUE(x == y) gtest will show what the values for the variables x and y are?

For example, in migraphx doing:

TEST_CASE(expect)
{
    int x = 1;
    int y = 2;
    EXPECT(x == y);
}

It will print out:

[   RUN    ] expect
void expect()
AMDMIGraphX/test/main.cpp:30:
    FAILED: x == y [ 1 == 2 ]
[  FAILED  ] expect: Test failure
[==========] 1 tests ran
[  FAILED  ] 1 tests failed
[  FAILED  ] expect

Does gtest show something similiar?

I also started refactoring activation.cpp for a simple "--float" test case, which may need further improvement to include the logic in test/CMakeLists.txt,

Is the plan to just have our test cases be gtests that just invoke the test driver? So we just keep the test driver and remove the --all option and lists in ctests?

Enumerating the tests in C++(with gtest or catch) does provide some nice filtering for running different subsets of tests. However, there is a compile-time cost that can grow as we add more tests or change existing tests.

A better option would be to have a json file with the different test configurations(its actually faster to parse json than to parse and compile C++). We would just need to provide our own filtering for this.

[atamazov]

@xinlipn

Yes, TEST_F() offers the same data configurations for different tests.

Can you please elaborate the machinery you are going to use in the fixture classes to enable the "re-using the data" feature in tests? For example, what is the expected lifetime of the instances of fixture classes (and, consequently, what is expected lifetime of the re-usable data they own)?

[xinlipn]

@pfultz2

Just curious, how does gtest handle passing parameters from CLI to tests?

Here's a thread discussing passing CLI parameters to gtest
google/googletest#765

[xinlipn]

So doing EXPECT_TRUE(x == y) gtest will show what the values for the variables x and y are?

I don't think so. With EXPECT_TRUE Macro, gtest spews diagnostic messages when the the condition is false, e.g.

EXPECT_TRUE(false) << "diagnostic message";

[xinlipn]

Accidentally changed status to closed. Sorry.

[xinlipn]

@xinlipn Thanks! Some other tidy issues before the static check can run through: http://micimaster.amd.com/blue/rest/organizations/jenkins/pipelines/MLLibs/pipelines/MIOpen/branches/sl%252Fgoogle_test/runs/20/nodes/97/steps/311/log/?start=0

such as

/var/jenkins/workspace/MLLibs_MIOpen_sl_google_test/test/gtest/cache.cpp:157:24: warning: Argument 'marker==1' to function EXPECT_TRUE is always 1 [constArgument]

@junliume , thank you for the reminder. This is actually caused by the code accidently checked in. It has been reverted. Yet CI doesn't seem to be triggered.

Please note the changes in Jenkinsfile is to force Docker to build a new container with update CMaker (3.11+). These changes should be reverted after the docker is created.

[junliume]

@xinlipn CI has passed, please ping all reviewers for their opinions. Thanks!

[xinlipn]

CI after retest has passed.

Could @JehandadKhan @atamazov @pfultz2 take another look? Thank you

http://micimaster.amd.com/blue/organizations/jenkins/MLLibs%2FMIOpen/detail/sl%2Fgoogle_test/27/pipeline

[junliume]

Ubuntu 18.04 has cmake 3.10. Would this potentially cause a problem if releasing team build from source? (I see in our own docker file we have installed 3.15, so where is 3.11 coming from?)

[xinlipn]

Hi @junliume , 3.11 is the minimal version that supports FetchContent. If Docker is used on Ubuntu 18.04, I believe the following changes in Dockerfile should cover the lower cMake version issue.

wget --no-check-certificate -qO - https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | apt-key add - &&
sh -c "echo deb https://apt.kitware.com/ubuntu/ bionic main | tee -a /etc/apt/sources.list" && \

[atamazov]

LGTM

[junliume]

@xinlipn please reopen another PR since this one is breaking develop branch.