Skip to content

Unfail anomaly detection tests#252

Open
nikie wants to merge 2 commits intoawslabs:masterfrom
nikie:unfail-anomaly-detection-tests
Open

Unfail anomaly detection tests#252
nikie wants to merge 2 commits intoawslabs:masterfrom
nikie:unfail-anomaly-detection-tests

Conversation

@nikie
Copy link
Copy Markdown

@nikie nikie commented Jun 6, 2025

Issue #, if available:
There are no user-facing changes.

Description of changes:

  • Unfail anomaly detection tests by removing pytest.mark.xfail marks and asserting the failing statements with self.assertRaises, except for the test_anomalyDetector test which relates to a not implemented feature.
  • Add assertions to the test_RelativeRateOfChangeStrategy test.
  • Apply black formatting to anomaly detection tests.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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


Generated by AI (model: us.anthropic.claude-opus-4-6-v1, prompt: 8c93b14f) — may not be fully accurate. Reply if this doesn't help.

@@ -473,57 +467,95 @@ def test_RelativeRateOfChangeStrategy(self):
[Row(check_status="Success")],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are now two methods named test_RelativeRateOfChangeStrategy in the same class. The first one (around line 408) tests various scenarios using the helper method self.RelativeRateOfChangeStrategy(...), and the second one (starting around line 476) is the newly uncommented test. Python will silently override the first with the second, so all the assertions from the first test_RelativeRateOfChangeStrategy (lines ~408-467) will never run. Rename one of them (e.g., test_RelativeRateOfChangeStrategy_detailed).

# can not implement breeze.stats.DescriptiveStats, because it is not an interface
# (breeze.stats.DescriptiveStats is in unnamed module of loader 'app')
@pytest.mark.xfail(reason="TODO: breeze.stats.DescriptiveStats is in unnamed module of loader 'app'")
def test_BatchNormalStrategy(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The @pytest.mark.xfail decorator was removed from test_BatchNormalStrategy, but the PR description says failing tests should use self.assertRaises. There is no assertRaises usage in this test. If the underlying breeze.stats.DescriptiveStats issue is actually fixed, this is fine, but if it still fails at runtime this will break CI instead of being a controlled expected failure.

# can not implement breeze.stats.DescriptiveStats, because it is not an interface
# (breeze.stats.DescriptiveStats is in unnamed module of loader 'app')
@pytest.mark.xfail(reason="TODO: breeze.stats.DescriptiveStats is in unnamed module of loader 'app'")
def test_holtWinters(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The @pytest.mark.xfail decorator was removed from test_holtWinters, but no self.assertRaises was added. Same concern as test_BatchNormalStrategy — if the breeze issue isn't actually resolved, this will cause unexpected test failures in CI.

# Todo: test anomaly detector
# Doesn't work in verification suite
# TODO: test anomaly detector
# Doesn"t work in verification suite
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The get_anomalyDetector method references self._jvm which is not defined on the test class (the class has self.spark from setup_pyspark). This should likely be self.spark._jvm.

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.

1 participant