OPENSCAP-4118 - Add script to build tests #13029

Mab879 · 2025-02-12T00:00:33Z

Description:

This is a very rough draft a script that renders the Automatus tests for a given product. The script will be clean up once the general form is agreed upon.

Rationale:

Make running the Automatus tests possible with out Automatus script.

Review Hints:

export ADDITIONAL_CMAKE_OPTION="-DSSG_BUILT_TESTS_ENABLED:BOOL=ON"
./build_product rhel10

openshift-ci · 2025-02-12T00:00:37Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Mab879 · 2025-02-14T20:59:27Z

Things left to do:

Check on Automatus runs. They seem to be broken.
Commands like ./automatus.py rule --datastream ../build/ssg-rhel10-ds.xml --libvirt qemu:///system automatus_rhel10 root_permissions_syslibrary_files are failing.
I need to doc how to turn it on.
Possible turn down the jobs number when building many products
Verify the correct tests are being ran

comps · 2025-02-14T21:17:29Z

build-scripts/build_tests.py

+    return (filename.endswith('.pass.sh')
+            or filename.endswith('.fail.sh') or
+            filename.endswith('.notapplicable.sh'))


str.endswith() takes a tuple, so you can probably just

Suggested change

return (filename.endswith('.pass.sh')

or filename.endswith('.fail.sh') or

filename.endswith('.notapplicable.sh'))

return filename.endswith(('.pass.sh', '.fail.sh', '.notapplicable.sh'))

comps · 2025-02-14T21:28:23Z

build-scripts/build_tests.py

+    for i in range(n):
+        end = start + chunk_size + (1 if i < remainder else 0)
+        inters.append(iter(items[start:end]))
+        start = end
+    return inters


If you don't mind interleaving the split, you could use python's array slicing "step" property.

Given 3 workers and a total of 10 rules, you can assign rules to them like this:

>>> a = [1,2,3,4,5,6,7,8,9,10] >>> a[0::3] [1, 4, 7, 10] >>> a[1::3] [2, 5, 8] >>> a[2::3] [3, 6, 9]

where the first (start) number is worker index, and the second (step) number is the total number of workers.

This is what we use in Contest to avoid any off-by-one problems and ensure no rule is left behind, while making things just simpler and evenly distributing rule complexity across all workers (as complex/simple rules tend to alphabetically bunch up).

https://github.com/RHSecurityCompliance/contest/blob/1204f812956a855eb15d26b1014cb28e5306ee05/per-rule/test.py#L93-L97

comps

I left some limited comments on the python code (as I am not familiar with the build system overall). Feel free to ignore any of them, as I don't know what is the level of code quality you want to maintain in this project.

Aside from the comments, consider (for an extra pedantic exercise):

typing consistency - most of the code seems untyped, but some functions have types despite being _ (a.k.a. internal, not part of API). I'm also not sure if done_file: pathlib.Path = output_path / ".test_done" does anything (with the inlined type)
Overall " vs ' consistency, they seem to be intermixed randomly
Lack of comments (especially in big functions like _process_rules()
Inconsistent indent (ssg.jinja.process_file_with_macros in _process_shared_file() seems to squash two arguments at the end, ssg.environment.open_environment in main() uses a different multi-line function call syntax.
f-strings for logger.* calls - note that you don't have to use the prehistoric %s syntax, if you pass only one argument, logging takes it as verbatim, so it can be an f-string like one of those you use for exceptions.

I'll test the functionality tomorrow, looking forward to it. 🙂

comps · 2025-03-06T21:18:24Z

build-scripts/build_tests.py

+def _get_deny_templated_scenarios(test_config_path):
+    deny_templated_scenarios = list()
+    if test_config_path.exists():
+        test_config = ssg.yaml.open_raw(str(test_config_path.absolute()))
+        if 'deny_templated_scenarios' in test_config:
+            deny_templated_scenarios = test_config['deny_templated_scenarios']
+    return deny_templated_scenarios


Maybe

Suggested change

def _get_deny_templated_scenarios(test_config_path):

deny_templated_scenarios = list()

if test_config_path.exists():

test_config = ssg.yaml.open_raw(str(test_config_path.absolute()))

if 'deny_templated_scenarios' in test_config:

deny_templated_scenarios = test_config['deny_templated_scenarios']

return deny_templated_scenarios

def _get_deny_templated_scenarios(test_config_path):

if test_config_path.exists():

test_config = ssg.yaml.open_raw(str(test_config_path.absolute()))

if 'deny_templated_scenarios' in test_config:

return set(test_config['deny_templated_scenarios'])

return set()

?

The .get() method is always shorter and in most of the cases faster:

Suggested change

def _get_deny_templated_scenarios(test_config_path):

deny_templated_scenarios = list()

if test_config_path.exists():

test_config = ssg.yaml.open_raw(str(test_config_path.absolute()))

if 'deny_templated_scenarios' in test_config:

deny_templated_scenarios = test_config['deny_templated_scenarios']

return deny_templated_scenarios

def _get_deny_templated_scenarios(test_config_path):

deny_templated_scenarios = set()

if test_config_path.exists():

test_config = ssg.yaml.open_raw(str(test_config_path.absolute()))

deny_templated_scenarios |= test_config.get('deny_templated_scenarios', [])

return deny_templated_scenarios

comps · 2025-03-06T21:36:20Z

build-scripts/build_tests.py

+    with open(shared_script_path, 'w') as file:
+        file.write(file_contents)
+        file.write('\n')


Suggested change

with open(shared_script_path, 'w') as file:

file.write(file_contents)

file.write('\n')

_write_path(file_contents, shared_script_path)

?

(And in other similar cases.)

comps · 2025-03-06T21:42:43Z

build-scripts/build_tests.py

+    root_path = pathlib.Path(args.root).resolve().absolute()
+    output_path = pathlib.Path(args.output).resolve().absolute()


.resolve() already makes the path absolute:

$ pydoc3 pathlib.Path.resolve | cat Help on function resolve in pathlib.Path: pathlib.Path.resolve = resolve(self, strict=False) Make the path absolute, resolving all symlinks on the way and also normalizing it.

I can clean that up.

comps · 2025-03-06T21:49:32Z

build-scripts/build_tests.py

+if __name__ == "__main__":
+    raise SystemExit(main())


I would honestly just let python interpreter defaults handle exit code - raise an exception if something bad happens, otherwise let it finish cleanly.

If you really want an exception-less exit on resolved_rules_dir not existing (seems like an arbitrary choice?), just do sys.exit(1) in that one case. sys will take care of the exit-related exception handling.

I can move to sys.exit. However, under the covers this is how sys.exit works.

I guess either way works. I remember some problems with raising SystemExit() with non-CPython intepreters, but we don't use those here, and those problems are probably fixed by now anyway. I would still leave the 0 exit to be done by default instead of SystemExit(0), but that's probably bikeshedding.

jan-cerny · 2025-03-07T08:47:31Z

build-scripts/build_tests.py

@@ -0,0 +1,230 @@
+#!/usr/bin/env python3


I don't like that the PR description says in rationale "Make testing with thin data streams possible." because testing with thin data stream has been possible for many months and people are testing with thin data streams daily. Please reword the rationale.

I have updated it.

jan-cerny · 2025-03-07T08:53:13Z

build-scripts/build_tests.py

@@ -0,0 +1,230 @@
+#!/usr/bin/env python3
+


This script is slow, it takes more than 2 minutes to build scenarios for RHEL 9. We need to think to speed up.

jan-cerny · 2025-03-07T08:59:11Z

build-scripts/build_tests.py

+    benchmark_cpes = _get_benchmark_cpes(env_yaml)
+
+    built_profiles_root = resolved_rules_dir.parent / "profiles"
+    rules_in_profiles = list(_get_rules_in_profile(built_profiles_root))


This should be a set because many rules are present in more than 1 profile so you have a lot of duplicates in this list. Converting to a set will automatically eliminate duplicates because each item can be only once in a set.

jan-cerny · 2025-03-07T09:06:17Z

build-scripts/build_tests.py

+        rule_root = rule_path.parent
+        rule_id = rule_root.name
+        if rule_id not in product_rules:
+            logger.debug("Skipping %s since it is not in the product", rule_id)


You don't have to open the resolved rule yaml file if it is skipped. You can get the rule id from the file name. It will save some time if you don't open resolved rules that aren't going to be processed.

jan-cerny · 2025-03-07T09:19:44Z

build-scripts/build_tests.py

+    processes = list()
+    for chunk in range(args.jobs):
+        process_args = (benchmark_cpes, env_yaml, output_path,
+                        all_resolved_rules[chunk::args.jobs], templates_root, rules_in_profiles,)


I would split the rules_in_profiles into chunks instead of all_resolved_rules because the all_resolved_rules contain rules that aren't going to be processed because they are not present in the built content so the subprocesses won't be load balanced. If you split rules_in_profiles to chunks each subprocess would get the same amout of rules to be processed, which would be better load balancing.

jan-cerny · 2025-03-07T09:49:44Z

build-scripts/build_tests.py

+                file_contents = ssg.jinja.process_file_with_macros(str(test.absolute()),
+                                                                   jinja_dict)
+                scenario = tests.ssg_test_suite.rule.Scenario(test.name, file_contents)
+                if scenario.matches_platform(benchmark_cpes):


This is very inefficient.

The method "matches_platform" loads all CPEs for all platforms. It calls common.matches_platform which calls common._get_platform_cpes which iterates over all matched products and loads all CPEs for these products.

Therefore for many test scenarios we load many CPEs, including CPEs for other products and we repeat the loading for each test scenario. The result is that this script spends most of the time in loading CPEs.

We need to create a different and simplified way for test scenarios platform matching here.

jan-cerny · 2025-03-07T09:54:15Z

build-scripts/build_tests.py

+            _write_path(file_contents, output_file)
+        file_contents = test.read_text()
+        scenario = tests.ssg_test_suite.rule.Scenario(test.name, file_contents)
+        if scenario.matches_platform(benchmark_cpes):


here we also use the problematic method

jan-cerny · 2025-03-07T09:58:19Z

build-scripts/build_tests.py

+    logger = logging.getLogger()
+    for test in rule_tests_root.iterdir():  # type: pathlib.Path
+        if not test.name.endswith(".sh"):
+            logger.debug("Skipping file %s in rule %s is it doesn't end with .sh.",


This will skip files like for example linux_os/guide/auditing/auditd_configure_rules/audit_file_modification/audit_rules_unsuccessful_file_modification/tests/test_audit.rules which are needed for the test scenarios to work.

There was some bad overrding of variables

jan-cerny · 2025-03-10T12:42:50Z

build-scripts/build_tests.py

+        if not line.startswith('#'):
+            # The loop is now in the main test content, stop processing the file.
+            break


The test scenario linux_os/guide/system/accounts/accounts-session/accounts_tmout/tests/multiline_profile_d.fail.sh which is marked as # platform = multi_platform_sle gets build when I build RHEL 9 content. It shouldn't be there, it's for SUSE. The reason seems to be that this test scenario has empty line before the # platform = multi_platform_sle line and therefore it's skipped by this code here.

jan-cerny · 2025-03-10T13:08:56Z

build-scripts/build_tests.py

+SSG_ROOT = str(pathlib.Path(__file__).resolve().parent.parent.absolute())
+JOB_COUNT = multiprocessing.cpu_count()
+T = TypeVar("T")
+


Now it's really fast! It takes me less than 3 seconds on my laptop to build RHEL 9 tests. Great improvement!

jan-cerny · 2025-03-10T13:11:23Z

build-scripts/build_tests.py

+    )
+    parser.add_argument(
+        "--resolved-rules-dir", required=True,
+        help="Directory with <rule-id>.yml resolved rule YAMLs"


missing space after YAMLs

jan-cerny · 2025-03-10T13:15:26Z

build-scripts/build_tests.py

+            _write_path(content, rule_output_path / test.name)
+
+
+def _process_rules(env_yaml: Dict, output_path: pathlib.Path,


Code Climate complains about code complexity of this function. Please split it to multiple functions. You can for example extract code related to templated tests to a new function _process_templated_tests which would be consistent with _process_local_tests.

jan-cerny · 2025-03-10T13:26:11Z

build-scripts/build_tests.py

+            deny_templated_scenarios = _get_deny_templated_scenarios(test_config_path)
+            for test in template_tests_root.iterdir():  # type: pathlib.Path
+                if not test.name.endswith(".sh") or test.name in deny_templated_scenarios:
+                    logging.warning("Skipping %s for %s as it is a denied test scenario",


jan-cerny · 2025-03-10T13:36:47Z

cmake/SSGCommon.cmake

+macro(ssg_build_tests PRODUCT)
+    add_custom_command(
+        OUTPUT "${CMAKE_BINARY_DIR}/${PRODUCT}/tests/.tests_done"
+        COMMAND env "PYTHONPATH=$ENV{PYTHONPATH}" "${PYTHON_EXECUTABLE}" "${CMAKE_SOURCE_DIR}/build-scripts/build_tests.py"  --build-config-yaml "${CMAKE_BINARY_DIR}/build_config.yml" --resolved-rules-dir "${CMAKE_CURRENT_BINARY_DIR}/rules" --output  "${CMAKE_CURRENT_BINARY_DIR}/tests" --product-yaml "${CMAKE_SOURCE_DIR}/products/${PRODUCT}/product.yml"


works fine for me 👍

codeclimate · 2025-03-10T21:42:54Z

Code Climate has analyzed commit fd08c5a and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 62.0% (0.0% change).

View more on Code Climate.

jan-cerny

Looks good to me now. It works for me as I expect.

@comps please check it if it's good for you

comps · 2025-03-11T19:24:31Z

Looks good to me, builds (at least on my RHEL-9) as intended.

I don't see any empty rule directories anymore (so I'm assuming rules without tests are not present inside build/rhel9/tests/), non-sh files are also included, no {{ or }} found via greps, it all looks legit.

I already used it to find quite a lot of "broken" tests, .sh scripts that don't have a shebang at the top (presumably Automatus calls them via bash script.pass.sh explicitly).

$ cd build/rhel9/tests/
$ (IFS=$'\n'; for f in $(find . -type f -name '*.sh'); do h=$(head -n1 "$f"); [[ $h != '#!/bin/bash' ]] && echo $f; done) | wc -l
266

Mab879 added the Infrastructure label Feb 12, 2025

Mab879 added this to the 0.1.77 milestone Feb 12, 2025

openshift-ci bot added the do-not-merge/work-in-progress label Feb 12, 2025

Mab879 force-pushed the build_tests branch from b3d5c66 to 2c37853 Compare February 13, 2025 20:16

comps reviewed Feb 14, 2025

View reviewed changes

comps mentioned this pull request Feb 16, 2025

Move building of content to python code RHSecurityCompliance/contest#343

Merged

Mab879 marked this pull request as ready for review February 25, 2025 21:57

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 25, 2025

Mab879 changed the title ~~DRAFT: Add script to build tests~~ Add script to build tests Feb 25, 2025

Mab879 requested review from comps and jan-cerny February 25, 2025 22:00

Mab879 changed the title ~~Add script to build tests~~ OPENSCAP-4118: Add script to build tests Feb 28, 2025

marcusburghardt changed the title ~~OPENSCAP-4118: Add script to build tests~~ OPENSCAP-4118 - Add script to build tests Mar 4, 2025

comps reviewed Mar 6, 2025

View reviewed changes

Mab879 added a commit to Mab879/content that referenced this pull request Mar 7, 2025

Apply recommendations from code review in ComplianceAsCode#13029

83cda50

Mab879 added a commit to Mab879/content that referenced this pull request Mar 7, 2025

Apply recommendations from code review in ComplianceAsCode#13029

2cb9282

Mab879 force-pushed the build_tests branch from 83cda50 to 2cb9282 Compare March 7, 2025 00:46

jan-cerny requested changes Mar 7, 2025

View reviewed changes

Mab879 force-pushed the build_tests branch from 801547d to a1eac0a Compare March 7, 2025 17:37

Mab879 added 8 commits March 7, 2025 11:42

Builds local tests

76dee7b

Add template support to build-scripts/build_tests.py

67604bf

Formatting and Jinja process commmon.sh.

96fca6c

Clean up variable name and scopes in build_tests.py

39bfe26

build_tests now uses multiprocessing

f47b79b

Update to use multliprocessing on build-scripts/build_tests.py

9a4f0cd

Clean up build-scripts/build_tests.py

8f9cec1

Reorder and clean up build-scripts/build_tests.py

b3a41e6

Mab879 added 16 commits March 7, 2025 11:42

Add build_tests to CMake

202e902

Adjust how product CPEs are done

6c6d81b

Clean up build_tests

bb124ad

Undo import changes in ssg_test_suite

03632d3

Move to using Python's built in chunking

0a0ba49

Clean up imports so build-scripts/build_tests.py works

d775539

Refactor and add logging to build-scripts/build_tests.py

d0221e9

Add docs for building tests

cff5359

Fix PEP8 issues

b5c1ca4

Code Clean Up in build-tests

cf4fbd0

Fix PEP8 in build-tests

9058298

Fix bug in build_tests.py

af4c525

There was some bad overrding of variables

Apply recommendations from code review in ComplianceAsCode#13029

c2ad834

Improve the speed of build_tests

3d2d737

Ensure all test files are copied

505747d

Improve formatting, logging, and comments in build_tests.py

abd17c3

Mab879 force-pushed the build_tests branch from a1eac0a to abd17c3 Compare March 7, 2025 17:42

jan-cerny self-assigned this Mar 10, 2025

jan-cerny reviewed Mar 10, 2025

View reviewed changes

Mab879 force-pushed the build_tests branch from b5aa398 to 94034d3 Compare March 10, 2025 21:10

Formatting and complexity fixes build-scripts/build_tests.py

fd08c5a

Mab879 force-pushed the build_tests branch from 94034d3 to fd08c5a Compare March 10, 2025 21:12

jan-cerny approved these changes Mar 11, 2025

View reviewed changes

comps mentioned this pull request Mar 11, 2025

Thin datastreams cannot be built outside of build_product #13163

Closed

jan-cerny merged commit a3eab6b into ComplianceAsCode:master Mar 12, 2025
110 of 111 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPENSCAP-4118 - Add script to build tests #13029

OPENSCAP-4118 - Add script to build tests #13029

Mab879 commented Feb 12, 2025 •

edited

Loading

openshift-ci bot commented Feb 12, 2025

Mab879 commented Feb 14, 2025

comps Feb 14, 2025

comps Feb 14, 2025

comps left a comment •

edited

Loading

comps Mar 6, 2025

evgenyz Mar 6, 2025

comps Mar 6, 2025

comps Mar 6, 2025

Mab879 Mar 6, 2025

comps Mar 6, 2025

Mab879 Mar 6, 2025 •

edited

Loading

comps Mar 6, 2025

jan-cerny Mar 7, 2025

Mab879 Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025

jan-cerny Mar 7, 2025 •

edited

Loading

jan-cerny Mar 10, 2025

jan-cerny Mar 10, 2025

jan-cerny Mar 10, 2025

jan-cerny Mar 10, 2025

jan-cerny Mar 10, 2025

jan-cerny Mar 10, 2025

codeclimate bot commented Mar 10, 2025

jan-cerny left a comment

comps commented Mar 11, 2025

		root_path = pathlib.Path(args.root).resolve().absolute()
		output_path = pathlib.Path(args.output).resolve().absolute()

		_write_path(content, rule_output_path / test.name)


		def _process_rules(env_yaml: Dict, output_path: pathlib.Path,

OPENSCAP-4118 - Add script to build tests #13029

OPENSCAP-4118 - Add script to build tests #13029

Conversation

Mab879 commented Feb 12, 2025 • edited Loading

Description:

Rationale:

Review Hints:

openshift-ci bot commented Feb 12, 2025

Mab879 commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comps left a comment • edited Loading

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

Mab879 Mar 6, 2025 • edited Loading

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

Choose a reason for hiding this comment

jan-cerny Mar 7, 2025 • edited Loading

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

codeclimate bot commented Mar 10, 2025

jan-cerny left a comment

Choose a reason for hiding this comment

comps commented Mar 11, 2025

Mab879 commented Feb 12, 2025 •

edited

Loading

comps left a comment •

edited

Loading

Mab879 Mar 6, 2025 •

edited

Loading

jan-cerny Mar 7, 2025 •

edited

Loading