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

fix: ssg_test_suite.get_product_context: fake Product #10582

Merged
merged 1 commit into from
May 18, 2023

Conversation

maage
Copy link
Contributor

@maage maage commented May 17, 2023

Description:

Product does not have setitem and here product_yaml is made as dict to mock real product.

But we want to change Product for testing. So just update and thus product_yaml type is not changed.

Traceback (most recent call last):
  File "/home/runner/work/content/content/tests/automatus.py", line 511, in <module>
    main()
  File "/home/runner/work/content/content/tests/automatus.py", line 507, in main
    options.func(options)
  File "/home/runner/work/content/content/tests/ssg_test_suite/rule.py", line 686, in perform_rule_check
    checker.test_target()
  File "/home/runner/work/content/content/tests/ssg_test_suite/oscap.py", line 683, in test_target
    self._test_target()
  File "/home/runner/work/content/content/tests/ssg_test_suite/rule.py", line 445, in _test_target
    rules_to_test = self._get_rules_to_test()
  File "/home/runner/work/content/content/tests/ssg_test_suite/rule.py", line 298, in _get_rules_to_test
    product_yaml = common.get_product_context(product)
  File "/home/runner/work/content/content/tests/ssg_test_suite/common.py", line 285, in get_product_context
    product_yaml['cmake_build_type'] = 'Debug'
TypeError: 'Product' object does not support item assignment
Error: Process completed with exit code 1.

Rationale:

Allow modify Product like at ssg_test_suite but not at ssg.

Review Hints:

See failed test at #10574

Product does not have __setitem__ and here product_yaml is made as dict
to mock real product.

But we want to change Product for testing. So just update and thus
product_yaml type is not changed.
@openshift-ci
Copy link

openshift-ci bot commented May 17, 2023

Hi @maage. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label May 17, 2023
@github-actions
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Mab879
Copy link
Member

Mab879 commented May 17, 2023

/packit build

@codeclimate
Copy link

codeclimate bot commented May 17, 2023

Code Climate has analyzed commit e3eab37 and detected 0 issues on this pull request.

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 52.5% (0.0% change).

View more on Code Climate.

@vojtapolasek vojtapolasek self-assigned this May 18, 2023
Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @maage and thank you for the quick fix. Althought I can confirm that the fix solves the problem, I think it would be better to equip the ssg.products.Product class with the proper setitem method instead. Your fix would create a difference in product_yaml in the test suite and product_yaml in the rest of the project. As the whole concept of accessing product properties is going to be changed, I think my suggestion will make later work easier by keeping the API the same.
Tagging @matejak because he introduced the Product class.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further investigation, I found out that the fix I propose is not solving the problem - the product_yaml is expected to be a dictionary in some other parts of the test suite as well, it leads to the part where some Jinja substitutions happen.. Due to the fact that currently no testing with Automatus is possible and this situation affects basically all newly created PRs, I am merging this PR although the proper solution should be different I believe. I will also create upstream issue pointing to the problem.

@vojtapolasek vojtapolasek merged commit d73adf7 into ComplianceAsCode:master May 18, 2023
@maage
Copy link
Contributor Author

maage commented May 18, 2023

Okay. Thank you for looking further.

I think tests/ssg_test_suite/common.py get_product_context should require product always. automatus seems to ensure test_env.product is set. And all calls for get_product_context come from tests/ssg_test_suite/rule.py and value as self.test_env.product, so essentially automatus rule. But I think that should be part of comprehensive lookout here.

This issue could have been catched by mypy as the variable has one type and at assignment value has another.

@vojtapolasek
Copy link
Collaborator

Just for posterity, the relevant issue is #10583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants