-
Notifications
You must be signed in to change notification settings - Fork 403
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
refactor(event_source): convert functional tests to unit tests #2506
refactor(event_source): convert functional tests to unit tests #2506
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #2506 +/- ##
===========================================
+ Coverage 97.20% 97.21% +0.01%
===========================================
Files 158 158
Lines 7358 7365 +7
Branches 534 537 +3
===========================================
+ Hits 7152 7160 +8
+ Misses 159 158 -1
Partials 47 47
☔ View full report in Codecov by Sentry. |
|
looking 👀 |
@leandrodamascena PR body can be improved - add a note about functional -> unit, before/after (functional -> unit change for a single test), etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so so happy we're finally getting this tech debt done - THANK YOU.
Two major changes before merging:
- We need a functional test for
@event_source
decorator - Split
raw_event
fromparsed_event
in every example, and assert whetherparsed_event.<property>
has the same data as theraw_event[<property>]
Can you check again please? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a hidden bug in ActiveMQ implementation of message
property when reviewing an older assertion it was doing.
We need to search for other uses of generators with next()
in properties, as it'll lead to the same problem.
|
4 similar comments
|
|
|
|
26ae39c
to
d8b2739
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 ⛴️
Just need to fix the typing for self._messages and records because they're being inferred as None, then assigned a generator later |
Issue number: #509
Summary
Changes
This pull request aims to enhance the test structure and organization within the
event_source_data_classes
module. By breaking down the existing tests into smaller, more focused units, this refactor improves the maintainability and readability of the codebase. Thetest_data_class.py file
had more than 2k LOC and it was becoming impossible to maintain it, so I divided it into small tests to make it easier to maintain.I've also changed function tests to unit tests, which makes more sense for the type of testing we're doing. Just to explain a little more, these tests specifically test whether given an event source, Powertools can turn it into an object.
User experience
There is no change in the user experience.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.