Optimize tests that run fix_rules.py #10968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change callback functions which name starts with `has_` so that these functions won't load YAML files inside them but instead they will consume a loaded YAML as a parameter. This will separate the logic from YAML parsing allow us to reuse these functions in a different context in future. The functions are used as a callback therefore they all need to have the same interface but that can lead to unused parameters in some of the functions.
This commit adds a new subcommand `test_all` to the `fix_rules.py` script. The subcommand will execute all checks that are present in this script on all rules in the product. In other words, it works like executing all currently existing subcommands in `--dry-run` mode at once. We currently call the subcomands of this script in `--dry-run` mode in our CTest test. This change will allow us to merge these tests in a single test case.
We currently have multiple CTest tests that run `fix_rules.py`. Each of the tests runs a different subcommand of `fix_rules.py`. These tests test various aspects of rule.yml files. Each test tests all rule.yml files in a product. That means that each test loads all rule.yml files to perform the check. This isn't efficient and takes a lot of time. Thanks to the new subcommand `test_all` that has been introduced to `fix_rules.py` recently, we can run all checks at once. Therefore, we will merge all the CTest tests that run `fix_rules.py` to a single test. This way, we will parse each rule.yml file just once, and that will save us a lot of time during testing.
|
Start a new ephemeral environment with changes proposed in this pull request: Fedora Environment Oracle Linux 8 Environment |
|
Code Climate has analyzed commit a429d02 and detected 4 issues on this pull request. Here's the issue category breakdown:
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 53.3% (0.0% change). View more on Code Climate. |
Mab879
approved these changes
Aug 10, 2023
|
I'm waving the code climate finings as this was how the code was before. We look to refactor this later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
We currently have multiple CTest tests that run
fix_rules.py, eg.fix_rules-empty_identifiers,fix_rules-invalid_identifiers,fix_rules-duplicate_subkeys. There is 8 of them and each runs for ~ 10 seconds.Each of the tests runs a different subcommand of
fix_rules.py. These tests test various aspects of rule.yml files. Each testtests all rule.yml files in a product. That means that each test loads all rule.yml files to perform the check. This isn't
efficient and takes a lot of time.
We will introduce a new subcommand
test_alltofix_rules.py. This subcommand can run all checks at once. It iterates over rules and on each rule it runs multiple checks. Each rule.yml is loaded and parsed just once.Therefore, we will merge all the CTest tests that run
fix_rules.pyto a single test. This way, we will parse each rule.yml file single time instead of 8 times during the test run, and that will save us a lot of time during testing.Rationale:
GitHub Actions CI speed up.
Review Hints:
Modify some rules so that the test is trigerred, for example, break ordering of items in prodtype key or break soorting of references in rule.yml. Then run
ctest --verbose -R fix_rulesinbuilddirectory.