Skip to content

Conversation

anisaoshafi
Copy link
Contributor

@anisaoshafi anisaoshafi commented Jan 22, 2025

Motivation

After merging this PR on eventbridge schedule validation, this integration test broke the pipeline: test_event_rule_creation_without_target.
That PR actually brought more parity with AWS and broke an existing test that shouldn't have passed.
Now the test is aws_validated 🚀

Changes

  • Fixed a cron schedule: in events_rule_without_targets.yaml. 'cron(0 1 * * * *)'--> 'cron(0 1 * * ? *)'
  • Test snapshot tested and aws validated.

@anisaoshafi anisaoshafi requested a review from bentsku January 22, 2025 14:23
@anisaoshafi anisaoshafi added aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases labels Jan 22, 2025
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for jumping on this 🦸‍♀️ great fix, nice seeing another test being validated and now understanding why it failed before. The comment on the invalid CRON is also very useful because it was hard to understand why it would fail!

Also kudos for adding the snapshot test 🚀 we can have even more confidence in the test now!

Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Awesome work @anisaoshafi and super fast fix 👏👏👏
Feel free to add a conditional skip for the EventBridge v1 provider

    @pytest.mark.skipif(
        is_old_provider(),
        reason="V1 provider does not support this feature",
    )

@anisaoshafi
Copy link
Contributor Author

@pytest.mark.skipif(
is_old_provider(),
reason="V1 provider does not support this feature",
)

Thanks a lot @joe4dev for sharing that hint. I was wondering why tests were failing on v1 provider.
I'll just push this condition 🙏🏼

@anisaoshafi
Copy link
Contributor Author

anisaoshafi commented Jan 22, 2025

@pytest.mark.skipif(
is_old_provider(),
reason="V1 provider does not support this feature",
)

Would you mind to briefly explain why these tests don't work on events version 1? 👀
There are three tests related to cron tests_put_rule_with_invalid_schedule_cron (the one that failed now), tests_put_rule_with_schedule_cron and test_event_rule_creation_without_target and I don't understand why these two didn't fail (or maybe didn't fail yet).🤔

@joe4dev
Copy link
Member

joe4dev commented Jan 22, 2025

@pytest.mark.skipif(
is_old_provider(),
reason="V1 provider does not support this feature",
)

Would you mind to briefly explain why these tests don't work on events version 1? 👀 There are three tests related to cron tests_put_rule_with_invalid_schedule_cron, tests_put_rule_with_schedule_cron and test_event_rule_creation_without_target and I don't understand why they didn't fail (or maybe didn't fail yet). Just tests_put_rule_with_invalid_schedule_cron 🤔

Sure, in the latest CircleCI run, I see 5 failing tests in v1 all parametrized versions of tests_put_rule_with_invalid_schedule_cron: https://app.circleci.com/pipelines/github/localstack/localstack/30752/workflows/d2b304d5-ca45-4cc1-90de-958587d187ea/jobs/273330/tests

I think this invalid schedule detection was just not implemented in v1 and somehow the v1 tests were not executed in the PR adding the tests here: https://github.com/localstack/localstack/pull/12156/files#diff-9f5c75eb50435206aabd8d5e75726710c0ae1da04aea56b14a51eacf2e82dde4R322

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 54m 50s ⏱️ +51s
3 997 tests +1  3 680 ✅ +2  317 💤 ±0  0 ❌  - 1 
3 999 runs  +1  3 680 ✅ +2  319 💤 ±0  0 ❌  - 1 

Results for commit e74d82c. ± Comparison against base commit 14b3a06.

@anisaoshafi anisaoshafi merged commit e5e74db into master Jan 22, 2025
31 checks passed
@anisaoshafi anisaoshafi deleted the fix-broken-test branch January 22, 2025 17:15
@anisaoshafi anisaoshafi added this to the 4.1 milestone Jan 23, 2025
@anisaoshafi anisaoshafi self-assigned this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants