-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ref(symbolicator): Add tests for LPQ tasks #29088
Conversation
f15c4a2
to
71e305f
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.
lgtm, though I have no idea how the mocking is supposed to work
# TODO: i am bad at python | ||
STORE |
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.
what were you trying to do? :-D
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.
pylance was telling me to use this :(
) | ||
|
||
|
||
STORE = create_store() |
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.
this is unfortunately an anti-pattern. You should make this a fixture:
@pytest.fixture
def store() -> RedisRealtimeMetricsStore:
return ...
And you'll find you won't need to go back to unittests' TestCase and .tearDown() below.
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 can make this a fixture, but i'm unsure how to guarantee that the functions i'm testing will be using that store returned by the fixture.
should scan_for_suspect_projects
and update_lpq_eligibility
take in an optional store
parameter and use that instead of whatever it imports from realtime metrics? or is it possible to return a fixture in a @mock.patch
?
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.
a potential workaround to ensure that test data isn't leaking between stores is to just modify the internally-stored prefix
on the store. if these tests are being run in parallel that might just cause race issues due to the store flip-flopping between different prefixes...
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 figured it out, pushed changes
e279541
to
6b25c64
Compare
21153ec
to
00e69b3
Compare
00e69b3
to
f4b4a9b
Compare
Adds in some basic tests for the LPQ tasks.