WIP: refactor/simplify test management #495

busykai · 2024-03-11T05:27:07Z

This PR replaces entire test_selector infrastructure to pick the next test. It separates the functionality of providing the next test to run into SandstoneTestSet class whose only contract is to respond to the question "what is the next test to run?". For this purpose, it provides an iterator which is used by opendcdiag to cycle through the tests. It ensures a cleaner interface to configure and run the tests. It also simplifies greatly the complexity and maintainability of the implementation. The behavior of the iterator is configurable to provide:

Randomness of the next test selection.
"Cycle though" behavior: an infinite iterator over the tests.
An option to ignore tests with unknown names.

This keeps it compatible with the previous implementation.

Additionally,

Current test count is kept globally and is not reset when the next iteration starts;
All special handling of mce_check test is removed from the main cycle and is encapsulated in the iterator instead.
The options to use built-in test list, test list file, as well as command-line options are now can be used together and combined. For example, an undesired test from a test list file can be disabled on the command-line using --disable option as opposed to editing the test list file needed before.

thiagomacieira · 2024-03-14T20:01:02Z

framework/sandstone.cpp

@@ -3181,6 +3211,8 @@ int main(int argc, char **argv)
    const char *on_crash_arg = nullptr;

    // test selection
+    std::vector<char *> enabled_tests;


const char * or std::string_view

thiagomacieira · 2024-03-14T20:01:22Z

framework/sandstone.cpp

        case disable_option:
-            disable_tests(test_set, optarg);
+            disabled_tests.push_back(optarg);
            break;
        case 'e':
-            add_tests(test_set, test_list, optarg);
+            enabled_tests.push_back(optarg);
            test_selection_strategy = Ordered;
            break;


Thank you, I've been meaning to do this for a while.

thiagomacieira · 2024-03-14T20:03:05Z

framework/sandstone.cpp

+    } else {
+        /* otherwise, start with all the applicable tests (self tests or
+         * regular. */
+        test_set = new SandstoneTestSet((sApp->shmem->selftest) ? SandstoneTestSet::SELF_TESTS : SandstoneTestSet::REGULAR_TESTS);


Sounds redundant with the init call above. Any way to simplify?

init creates a catalog of all known tests, constructor is used to creates a instance which contains the tests to be run in this execution of opendcdiag. That's the difference. The reason it's not simply boolean in the constructor is that we need to start with an empty set at times (e.g. when -e is specified on the command line).

thiagomacieira · 2024-03-14T20:03:43Z

framework/sandstone.cpp

@@ -3246,6 +3278,7 @@ int main(int argc, char **argv)
        case 'l':
        case raw_list_tests:
        case raw_list_groups:
+            test_set = new SandstoneTestSet(SandstoneTestSet::REGULAR_TESTS);


This means we can't list the selftests. Please don't remove that ability.

thiagomacieira · 2024-03-14T20:04:58Z

framework/sandstone.cpp

+    // If we want to use the weighted testrunner we need to initialize it
+    if (test_list_file_path) {
+        if (builtin_test_list_name)
+            logging_printf(LOG_LEVEL_QUIET,
+                           "# WARNING: both --test-list-file and --use-builtin-test-list "
+                           "specified, using test file \"%s\".\n", test_list_file_path);
+        if (test_list.size()) {
+            logging_printf(LOG_LEVEL_QUIET,
+                           "# WARNING: both --test-list-file and --enable specified, using only "
+                           "the test list file \"%s\".\n", test_list_file_path);
+            test_list = {};
+        }


Shouldn't this be above the enable/disable code above?

thiagomacieira · 2024-03-14T20:06:21Z

framework/sandstone_tests.cpp

+static std::map<const char *, struct test *, SandstoneTestSet::cstr_cmp> all_test_map; /* maps test name to test */
+static std::map<const char *, std::vector <struct test *>> all_group_map; /* maps group name to a vector of tests it contains */


Please don't use std::map globals (they have an init function). Move these to inside SandstoneApplication.

Use std::string_view as the key type instead of const char *, so you don't have to have cstr_cmp.

thiagomacieira · 2024-03-14T20:06:42Z

framework/sandstone_tests.cpp

+        case SELF_TESTS:
+            known_tests = selftests;
+            break;


#ifndef for no-self-tests

thiagomacieira · 2024-03-14T20:07:37Z

framework/sandstone_tests.cpp

+        try {
+            struct test *t = all_test_map.at(name);
+            res.push_back(t);
+        } catch(const std::out_of_range &e) {
+        }


Don't use the exception-throwing lookup. Use find instead.

framework/sandstone_tests.h

framework/sandstone_tests.cpp

The framework is broken, but clean of what we will be getting rid of. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

This is to support --test-list-randomize and various timing options when OpenDCDiag cycles through the list. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

Instead of reinitializing SMI counts every "loop" of the test, it's done after smi_count test has been run (in its clean up). Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

Previously it was not "injected" at the very end if there were failured, but it was injected inbetween regardless. With the new implementation it is consistently run at the end of each iteration. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

The mce_test (mce_check) is now added to the end of the test_set. The logic of SandstoneTestSet will keep it last on the list. It will be therefore run each iteration. Code that resets the count on new iteration as well as printing of the header is moved to the iterator instead of main cycle. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

selftest_sigkill is not a valid test name on Windows. After the refactoring opendcdiag.exe produces an empty output when an illegal test name is specified. Before it was not touching the log file, but the test was failing cause it was reading the output.yaml of the previous run (note the loop over tests). Also, make the output file unique. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

busykai force-pushed the kai/refactor-test-management branch 6 times, most recently from ad28ae3 to f8c3dbc Compare March 14, 2024 06:29

thiagomacieira requested changes Mar 14, 2024

View reviewed changes

busykai force-pushed the kai/refactor-test-management branch 2 times, most recently from b94a4da to bb448d6 Compare March 21, 2024 15:38

busykai force-pushed the kai/refactor-test-management branch 8 times, most recently from f19f6b2 to be87b1f Compare April 9, 2024 23:21

busykai force-pushed the kai/refactor-test-management branch 7 times, most recently from c0ac468 to 941e025 Compare April 17, 2024 22:58

busykai force-pushed the kai/refactor-test-management branch 5 times, most recently from 553ad70 to 6e99b43 Compare May 3, 2024 02:30

busykai force-pushed the kai/refactor-test-management branch from 605be56 to 32e82de Compare May 7, 2024 23:25

busykai marked this pull request as ready for review May 8, 2024 16:02

busykai added 12 commits May 9, 2024 05:22

Remove the entire test_selectors code.

233c851

The framework is broken, but clean of what we will be getting rid of. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Refactor test management.

3d23d5a

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Add support for test lists.

0737c0e

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Configure TestSet when creating.

1c6f7cc

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Implement randomness and infinite iterator.

21ab5c1

This is to support --test-list-randomize and various timing options when OpenDCDiag cycles through the list. Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Use global state to keep the test count.

46e5023

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Refactor how SMI accounting is done.

cab5150

Instead of reinitializing SMI counts every "loop" of the test, it's done after smi_count test has been run (in its clean up). Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Add support for builtin test lists.

e2f79d9

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

FW: Test mgmt refactor: Separate test list and lifecycle management.

27213df

Signed-off-by: Arzhan Kinzhalin <arzhan.i.kinzhalin@intel.com>

busykai force-pushed the kai/refactor-test-management branch from 32e82de to 2fe7042 Compare May 9, 2024 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: refactor/simplify test management #495

WIP: refactor/simplify test management #495

busykai commented Mar 11, 2024 •

edited

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

busykai Mar 14, 2024

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

thiagomacieira Mar 14, 2024

		static std::map<const char , struct test , SandstoneTestSet::cstr_cmp> all_test_map; /* maps test name to test */
		static std::map<const char , std::vector <struct test >> all_group_map; /* maps group name to a vector of tests it contains */

WIP: refactor/simplify test management #495

Are you sure you want to change the base?

WIP: refactor/simplify test management #495

Conversation

busykai commented Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busykai commented Mar 11, 2024 •

edited