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

Update global tests (part 1) #99

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

RadWolfie
Copy link
Member

Instead of contributors manually input strings of hex index / name of api. I simply replace all into parameters to remove any possible typo mistakes. This way, the compiler/runtime will do the work for us easier.

Instead of manually updating each one which will take hours. I used a couple of regexes to replace all of it in one go. It only took me about 15 seconds and 15 minutes to replace what I had missed. I also spent 45 minutes verifying that each api test bodies were correctly replaced.

In this change:

  • void test_<name of api>(void) been replaced to void test_<name of api>(int func_num, const char* func_name)
    • With this replacement, local variables of func_num and func_name of all tests had been removed.
  • renamed kernel_thunk_table to kernel_api_tests per @PatrickvL suggestion request
  • renamed func_table.h to func_tests.h

I also confirmed all implemented tests did output the correct hex index value in the log file.

@PatrickvL
Copy link
Member

Nice work, so consider the following to be less important than what's already been done here:

If there's a chance the test function signature have to change again, then perhaps it's handy to define a macro for it?
That way, if the arguments passed into each test need to change, only the macro would have to be updated.
And if that's combined with macros for the printing functions as well (the ones that currently use the test arguments) then also the printing can be redeclared centrally, without having to alter all tests.

Of course, careful consideration is needed whether macros are acceptable for such an application...

@PatrickvL
Copy link
Member

#define TEST_FUNC(name) void name(int api_num, char *api_name)

TEST_FUNC(RtlLower)
{
  // ...
  TEST_FINISHED();
}

@RadWolfie
Copy link
Member Author

RadWolfie commented Feb 19, 2024

Hmm, you're missing the starting of the test function aka print_test_header(func_num, func_name);.

So it would be something like...

#define TEST_FUNC(name) void name(int api_num, char* api_name)
#define TEST_START() print_test_header(api_num, api_name); \
    BOOL test_passed = 1
#define TEST_FINISHED() print_test_footer(api_num, api_name, test_passed)

TEST_FUNC(RtlLower)
{
    TEST_START();
    // ...
    TEST_FINISHED();
}

However, there are some other tests that are using tests_passed instead of test_passed name. Meaning those will need to replace as well.

@RadWolfie
Copy link
Member Author

Then the func_tests.h will be better to rename as api_tests.h, correct? In order to reflect the changes from func_<num|name> to api_<num|name>.

@PatrickvL
Copy link
Member

PatrickvL commented Feb 19, 2024

Or you could pass that in as an argument to that macro?

@RadWolfie
Copy link
Member Author

RadWolfie commented Feb 19, 2024

Or you could pass that in as an argument to that macro?

Good question, should we? Since we have GEN_ prefix defines that are locked to local test_passed variable which aren't passed as argument in the macros. If you do feel TEST_FINISHED should pass test boolean as argument, then I will accept it anyway.

@PatrickvL
Copy link
Member

Or you could pass that in as an argument to that macro?

Good question, should we? Since we have GEN_ prefix defines that are locked to local test_passed variable which aren't passed as argument in the macros. If you do feel TEST_FINISHED should pass test boolean as argument, then I will accept it anyway.

The test_passed variable is declared once via the define ASSERT_HEADER and a few times independently (but those could all use the ASSERT_HEADER). [Nearly all test_passed reads/writes are similar, and could be done via macro's as well, but it feels a unnecessary to start using macro's for otherwise simple declarations.]

Like test_passed, the func_num and func_name are also sometimes used (mostly by a print_test_footer call), but those could also all be moved inside a define (if that's the way to go).

So, perhaps the print_test_header and print_test_footer calls could be wrapped in a macro, so that the func_num and func_name variables no longer appear in the actual test code (only in the macro's, including the proposed TEST_FUNC macro.)

As for when to pass in an argument to a macro, and when to just use it in the macro (assuming the symbol/argument/variable will exist), I think that for func_num and func_name they should not even appear in the code anymore (so only be mentioned in macro declarations) - likely best done by moving the print_test_header and print_test_footer into macro's. With that, the amount of mentions of test_passed is reduced significantly, and quite a few of the ones that remain are inside ASSERT macro's already. So don't keeping around a few mentions of test_passed in test code seems not worth the trouble to move it into a macro.

Besides, assert_NTSTATUS always appears like this : tests_passed &= assert_NTSTATUS(result, ... , func_name); and could thus incorporate the tests_passed &= part, which would remove many of the remaining tests_passed occurences too.

@RadWolfie
Copy link
Member Author

Actually, we could move test_passed &= into pre-existing macros. As for the manual check such as:

if(ret == 1) {
    test_passed &= 1;
}
else {
    test_passed &= 0;
}

can be convert to use GEN_CHECK macro.

For any custom checks in the tests that aren't using pre-existing macros. We could make additional macro specifically for this purpose. Which could be TEST_FAILED() macro as test_passed = 0.
Then test_passed become completely hidden from all of api test's body. I can also add a comment to tell creator of the test not to use the variable for their own purpose.

Though, for ^ purpose it can possibility take a while to find and replace them. Which will make it harder to review in this pull request. I would like to handle this test(s)_passed replacement in a separate pull request if you don't mind.

@RadWolfie RadWolfie changed the title Update global tests Update global tests (part 1) Feb 20, 2024
@RadWolfie
Copy link
Member Author

RadWolfie commented Feb 20, 2024

Finalize discussion changes has been pushed recently. Of course excluding minor changes for part 2 pull request mainly to have better readability review after this majority changes.

@RadWolfie RadWolfie merged commit e91ed08 into Cxbx-Reloaded:master Feb 21, 2024
1 check passed
@RadWolfie RadWolfie deleted the update-global-tests branch February 21, 2024 07:32
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