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

Establishing naming conventions for test macros #1388

Open
Xrayez opened this issue Aug 19, 2020 · 1 comment
Open

Establishing naming conventions for test macros #1388

Xrayez opened this issue Aug 19, 2020 · 1 comment

Comments

@Xrayez
Copy link
Contributor

Xrayez commented Aug 19, 2020

Describe the project you are working on:
Goost - Godot Engine Extension.

Describe the problem or limitation you are having in your project:
Inconsistency between error macros and test macros names, in terms of usage.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Provide aliases for doctest macros to follow Godot naming conventions.

If you look at core/error_macros.h, you'll see a set of macros which are currently used by the engine for handling errors. All are prefixed with ERR_*, and some have corresponding *_MSG macros for conveying error rationale.

doctest provides a default set of simple macros such as CHECK and REQUIRE (which are also aliases for DOCTEST_CHECK and DOCTEST_REQUIRE internally, by the way). Macros with rationale are quite verbose too, for instance: CHECK_MESSAGE.

Currently we have tests/test_macros.h which could also have similar macros as in core/error_macros.h defined specifically for testing. They would be prefixed with TEST_* and also have corresponding *_MSG macros.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
Here's a rename mapping I propose, feel free to suggest anything else.

Assertions

Sorted by severity (prefer CHECK macros as they do not fail the test immediately).

doctest Godot Description
DOCTEST_REQUIRE TEST_REQUIRE Test condition, fail the test if does not hold true.
DOCTEST_REQUIRE_MESSAGE TEST_REQUIRE_MSG Same as above, prints a rationale.
DOCTEST_REQUIRE_FALSE TEST_FAIL_COND Fail the test if condition holds true.
DOCTEST_REQUIRE_FALSE_MESSAGE TEST_FAIL_COND_MSG Same as above, prints a rationale.
DOCTEST_CHECK TEST_CHECK Simple assertion.
DOCTEST_CHECK_MESSAGE TEST_CHECK_MSG Same as above, prints a rationale.
DOCTEST_CHECK_FALSE TEST_COND Test if condition holds true, but does not fail the test.
DOCTEST_CHECK_FALSE_MESSAGE TEST_COND_MSG Same as above, prints a rationale.
DOCTEST_WARN TEST_WARN Test condition, does not fail the test, but logs a warning if something does not hold true.
DOCTEST_WARN_MESSAGE TEST_WARN_MSG Same as above, prints a rationale.
DOCTEST_WARN_FALSE TEST_COND_WARN Test condition, does not fail the test, but logs a warning if something holds true.
DOCTEST_WARN_FALSE_MESSAGE TEST_COND_WARN_MSG Same as above, prints a rationale.

Note that TEST_COND and TEST_FAIL_COND are already used locally in ClassDB tests, for instance.

Logging

doctest Godot Description
DOCTEST_MESSAGE TEST_MSG Print a message.
DOCTEST_FAIL_CHECK TEST_FAIL_CONTINUE Mark the test as failing, but continue the execution. Can be wrapped in conditionals for complex checks.
DOCTEST_FAIL TEST_FAIL Fail the test immediately. Can be wrapped in conditionals for complex checks.

I'd like to refactor all existing macros usages once we reach a consensus.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
C++ developers can still choose to use their own macros however they see it, or continue using doctest macros.

Is there a reason why this should be core and not an add-on in the asset library?:
Have to modify tests/test_macros.h.

@Calinou
Copy link
Member

Calinou commented Sep 26, 2021

I think the current names we use for Doctest macros are fine (CHECK_MESSAGE(), etc). We don't need to be very consistent as the unit tests' code is separated from the engine code.

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

No branches or pull requests

2 participants