Skip to content
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 notifications checks in tests #657

Merged
merged 1 commit into from
May 3, 2024

Conversation

santib
Copy link
Collaborator

@santib santib commented May 3, 2024

  • Unify how notifications are tracked and checked using Array's #push #pop and #size.
  • With this now we not only check "some notification happened" but the exact amount of notifications since in some cases we might trigger more than once, so this can prevent missing or mixing notifications.

Note: We could introduce a custom class built on top of Array with some facilities but seems to not be worth it at the time

@santib santib requested a review from grzuy May 3, 2024 18:23
Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Good call.
Neat.

Maybe adding some array emptying on after each test hook?
Just not to rely only on each test poping?
Minor thing, your call.

@santib
Copy link
Collaborator Author

santib commented May 3, 2024

Good call. Neat.

Maybe adding some array emptying on after each test hook? Just not to rely only on each test poping? Minor thing, your call.

Thanks for the review 😊 I think the emptying isn’t needed because the array gets recreated for each test. Or am I missing something?

@grzuy
Copy link
Collaborator

grzuy commented May 3, 2024

Good call. Neat.
Maybe adding some array emptying on after each test hook? Just not to rely only on each test poping? Minor thing, your call.

Thanks for the review 😊 I think the emptying isn’t needed because the array gets recreated for each test. Or am I missing something?

Right, hehe.
My bad.

@santib santib merged commit 52ce45d into rack:main May 3, 2024
119 checks passed
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