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

Add tests for D&O merge strategy #638

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

averevki
Copy link
Contributor

Refined part of #608 with merge strategy tests only

Closes #566

@averevki averevki added the Test case New test case label Feb 18, 2025
@averevki averevki self-assigned this Feb 18, 2025
@trepel
Copy link
Contributor

trepel commented Mar 6, 2025

I don't like two additional things:

  1. In the bodies of test functions the policy status is not asserted (whether policy is indeed overridden/fully and/or partially enforced).
  2. The commit fixture is overloaded so that it only check for policy acceptance. I think (this is philosophical and up to wider discussion maybe) that it is better if commit fixture only deals with policies that are fully enforced (I consider this as initial test setup fixtures should be responsible for because that is what they are designed for) and policies that are overridden/partially enforced (or causes some existing policies to get overridden/partially enforced) should be created in test function bodies (I consider this test scenario rather that initial test setup so it does not belong to fixtures anymore).

@averevki averevki force-pushed the defaults_overrides_merge_strategy branch from 961870d to 5804d44 Compare March 10, 2025 19:18
@averevki
Copy link
Contributor Author

In the bodies of test functions the policy status is not asserted (whether policy is indeed overridden/fully and/or partially enforced).

good idea, done

The commit fixture is overloaded so that it only check for policy acceptance. I think (this is philosophical and up to wider discussion maybe) that it is better if commit fixture only deals with policies that are fully enforced (I consider this as initial test setup fixtures should be responsible for because that is what they are designed for) and policies that are overridden/partially enforced (or causes some existing policies to get overridden/partially enforced) should be created in test function bodies (I consider this test scenario rather that initial test setup so it does not belong to fixtures anymore).

agreed, there are a lot of other tests doing that inside the testsuite though. We can do another single PR with all these changes in the future

Copy link
Contributor

@trepel trepel left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks!

@averevki averevki merged commit 11849aa into Kuadrant:main Mar 12, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Test case New test case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaults & Overrides: Merge strategy tests
2 participants