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

Add unit tests #1862

Merged
merged 8 commits into from
Apr 30, 2024
Merged

Add unit tests #1862

merged 8 commits into from
Apr 30, 2024

Conversation

smilediver
Copy link
Contributor

@smilediver smilediver commented Apr 28, 2024

This PR adds unit tests by introducing a new unit-tests app.

Why?

Right now the only way to test the engine is either by hands or cpp-tests, which is an unusable nightmare. It's not automated, with bad and slow UX, you have no idea what the tests are testing or how they are supposed to look, etc. The first and a primary tool for non visual tests should be unit tests. It must be fully automatic, fast, easy to use and easy to add and write new tests.

How?

This PR adds unit-tests app. It's a console app that automatically runs a bunch of tests and at the end prints out if all tests pass. If tests do not pass, it shows which fail, give you source file location for the failing test and gives the reasons why it fails. The app also uses exit codes to signal if tests pass, so this can be used in a CI/CD environments.

The app itself uses doctest for writing unit tests. It's pretty simple, but has a lot of nice features. I've looked for something simple to use, and that would integrate easily. Looked at several alternatives, but they either were not maintained anymore or were too complex, and doctest was the only choice.

How it helps me?

You can develop new engine features or fix bugs by simply working on unit-tests project. You write some code, add tests for it and instantly know if your new code works or if it breaks some existing functionality. This will improve iteration times, and allow you to be sure that you're not breaking something else in the process.

How do the tests look like?

I've converted some tests from cpp-tests, you can see them at tests\unit-tests\Source\core. I recommend checking tests\unit-tests\Source\core\platform\FileUtilsTests.cpp, since it's closest to how ideal tests should looks like. Quick example:

    TEST_CASE("standard_url") {
        std::string s("http://www.facebook.com/hello/world?query#fragment");
        Uri u = Uri::parse(s);
        CHECK_EQ("http", u.getScheme());
        CHECK_EQ("www.facebook.com", u.getHost());
        CHECK_EQ(80, u.getPort());
        CHECK_EQ("www.facebook.com", u.getAuthority());
        CHECK_EQ("/hello/world", u.getPath());
        CHECK_EQ("query", u.getQuery());
        CHECK_EQ("fragment", u.getFragment());
        CHECK_EQ(s, u.toString());  // canonical
    }

How does running tests looks like?

Success!

[doctest] test cases:  50 |  50 passed | 0 failed | 1 skipped
[doctest] assertions: 975 | 975 passed | 0 failed |
[doctest] Status: SUCCESS!

Failure...

D:\work\axmol\tests\unit-tests\Source\core\network\UriTests.cpp(34):
TEST SUITE: network/Uri
TEST CASE:  standard_url

D:\work\axmol\tests\unit-tests\Source\core\network\UriTests.cpp(40): ERROR: CHECK_EQ( "www.facebook.com", "www.badbook.com" ) is NOT correct!
  values: CHECK_EQ( www.facebook.com, www.badbook.com )

===============================================================================
[doctest] test cases:  50 |  49 passed | 1 failed | 1 skipped
[doctest] assertions: 975 | 974 passed | 1 failed |
[doctest] Status: FAILURE!

Future

  • I hope we will have a rule that all bugfixes, new or touched features should come with unit tests.
  • And I also hope, that time from time we will move other unit test like tests from cpp-tests to unit-tests.
  • I'm also working on gfx-tests app to automate the graphics side of things.

Status

  • For now targeting only desktop OS'es, but in the future it would be nice to add support for mobiles too.
  • Tested on Windows and macOS.
  • It would be nice if someone could test it and fix it on Linux, I don't have access to it.
  • I have moved some tests from cpp-tests as a first step.
  • tests\unit-tests\Content is included in the repo. Right now it contains only 3 tiny text files, but I imagine in the future it might contain some larger test assets. So it might be a good idea to setup a separate repo to hold the assets. We can do it now or later.

@smilediver
Copy link
Contributor Author

Tested on macOS and it works apart some tests failing.

Made a PR for tests\unit-tests\Content assets: axmolengine/axmol-sample-assets#1 . After assets are merged in I'll make relevant changes for unit-tests to use them.

@smilediver
Copy link
Contributor Author

Changed tests\unit-tests\Content to take the assets from axmol-sample-assets repo. So I guess it's ready for merging.

It would be nice if someone could test it if it works on Linux.

@halx99 halx99 added this to the 2.1.3 milestone Apr 30, 2024
Copy link
Collaborator

@halx99 halx99 left a comment

Choose a reason for hiding this comment

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

I think, you can add the target unit-tests to main CMakeLists.txt like cpp-tests and build it in github ci like: https://github.com/axmolengine/axmol/blob/dev/.github/workflows/build.yml#L100

EDIT: since unit-tests is a console app, you can also run it after build success on github ci

@smilediver
Copy link
Contributor Author

Added to the workflows, but lets not enable test running for now, because not all tests pass (issues like #1863). Lets merge this PR first, then I'll make a separate PR to fix the tests and enable the running.

@halx99
Copy link
Collaborator

halx99 commented Apr 30, 2024

Added to the workflows, but lets not enable test running for now, because not all tests pass (issues like #1863). Lets merge this PR first, then I'll make a separate PR to fix the tests and enable the running.

merged

@smilediver
Copy link
Contributor Author

merged

Maybe you misunderstood me... I've meant lets merge this PR (#1862) first, then I'll make the fixes and enable running unit tests in a separate PR.

@halx99
Copy link
Collaborator

halx99 commented Apr 30, 2024

seems all intel cpu desktop contains macos, windows, linux build will error

@halx99 halx99 merged commit 9ac6db4 into axmolengine:dev Apr 30, 2024
11 of 15 checks passed
@halx99
Copy link
Collaborator

halx99 commented Apr 30, 2024

ok, let's merge first

@smilediver smilediver deleted the unit-tests branch April 30, 2024 17:03
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.

2 participants