Skip to content

feat: implement rules customization unit tests using feature files - #206

Open
sinapah wants to merge 7 commits into
feat/rules-customizationfrom
feat/rules-customization-feature-files
Open

sinapah wants to merge 7 commits into
feat/rules-customizationfrom
feat/rules-customization-feature-files

Conversation

@sinapah

@sinapah sinapah commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Issue

Implements the unit tests in #205 using feature files.

Solution

  • Dependencies: Added pytest-bdd to dev dependencies to enable Gherkin feature files as the test driver.

  • Feature Files: Created tests/features/alert_rule_customization.feature containing 20 behavioral scenarios:

  • Removal target rules: By alert name, group name, label, annotation, and topology label.

  • Patch updates:** For duration, alert name, expr, labels, and annotations.

  • Rule management: Adding rules under the fixed key.

  • Core properties: Input immutability, operation ordering, and instance reusability.

  • BDD Tests: Added tests/test_rules_customization.py to bind pytest-bdd step definitions to the feature file, replacing the legacy all-in-one test module.

  • Schema Tests: Added tests/test_rules_customization_schema.py (extracted from the old module) to isolate schema validation and no-op config tests, keeping complex bad-input matrices in clean unittest cases.

  • Shared Fixtures: Added tests/conftest.py containing the sample_alerts fixture and find_rule helper function for use across test suites.

Checklist

  • I have added or updated relevant documentation.
  • PR title makes an appropriate release note and follows conventional commits syntax.
  • Merge target is the correct branch, and relevant tandem backport PRs opened.
  • This change warrants a release, so I have updated the project.version field in the pyproject.toml file.

Context

Testing Instructions

Upgrade Notes

Given a set of relation alerts from two apps

# ---------------------------------------------------------------------------
# Remove

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should remove, patch be covered in separate feature files?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Not just that, but I think now that we've decided to do away with add, we can have three feature files: one for each of patch, remove, and remove+patch`.

# ---------------------------------------------------------------------------

Scenario: Remove an alert by name
When I apply a customization that removes alert "LowThroughput"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • This seems to suggest that the feature file isn't self-contained - there's a implicit dependency on some python code to make "LowThroughput" available. Not sure, but seems like an antipattern. Should we try to make the feature file more self contained? E.g. using the docstring fixture.
  • I think there is room for making greater use of parsers.parse().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. You're right in that the feature file isn't fully self-contained. However, this seems like (more or less) normal BDD behaviour? In the case of alerts, passing them as docstrings to the set up functions would make the feature file very verbose and it would considerably decrease readability.
  2. Yes, let's use parsers.parse().

@sinapah sinapah mentioned this pull request Aug 26, 2026
4 tasks
@sinapah
sinapah marked this pull request as ready for review August 26, 2026 19:35
@sinapah
sinapah requested a review from a team as a code owner August 26, 2026 19:35
Comment thread tests/conftest.py


@pytest.fixture
def sample_alerts():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not inline this (and others) in the feature file's docstring?
https://pytest-bdd.readthedocs.io/en/latest/#scenario-outlines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was the original unit testing added. Since we've revamped the testing to rely on Gherkin, we can scrap it.

Comment thread tests/conftest.py
Comment on lines +36 to +41
@given(parsers.parse('the sample alerts from "{filename}"'))
def given_sample_alerts_from_file(ctx, filename):
path = _HERE / filename
with open(path) as f:
ctx["alerts"] = yaml.safe_load(f)
ctx["original"] = copy.deepcopy(ctx["alerts"])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use a target fixture, look at https://pytest-bdd.readthedocs.io/en/latest/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants