Skip to content

[WPB-8881] Add unit tests for new effect actions#4331

Merged
mdimjasevic merged 8 commits intodevelopfrom
wpb-8881/effect-action-unit-tests
Nov 6, 2024
Merged

[WPB-8881] Add unit tests for new effect actions#4331
mdimjasevic merged 8 commits intodevelopfrom
wpb-8881/effect-action-unit-tests

Conversation

@mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Nov 5, 2024

This is a follow-up PR to #4316 that adds unit tests for effect actions that were introduced in that PR.

Tracked by https://wearezeta.atlassian.net/browse/WPB-8881

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@mdimjasevic mdimjasevic requested a review from fisx November 5, 2024 15:31
@echoes-hq echoes-hq bot added echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. labels Nov 5, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 5, 2024
@mdimjasevic mdimjasevic force-pushed the wpb-8881/effect-action-unit-tests branch from a8b9f2d to 458d5e0 Compare November 5, 2024 16:19
@mdimjasevic mdimjasevic force-pushed the wpb-8881/effect-action-unit-tests branch from 416c464 to fda4af8 Compare November 6, 2024 13:17
@mdimjasevic
Copy link
Contributor Author

@supersven , is there anything else you'd like me to change, update, or do about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered to add a test where the lookup fails? E.g. there is a code, but the key does not match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added it! I've rebased and pushed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks! (The more test code coverage we can add, the better - as long as the task doesn't grow out of bounds, of course 😄 )

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it wouldn't be useful to also test what happens when the user does not exist. Or, can't this case not happen? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that can happen. I'd argue it's out of scope for this ticket. Lots of stuff hasn't been tested, and I've added tests for the actions that I introduced in that original PR. Looking up a user in the storage is not an action that was in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Then, I'm approving. 👍

@mdimjasevic mdimjasevic force-pushed the wpb-8881/effect-action-unit-tests branch from fda4af8 to bca9dea Compare November 6, 2024 13:49
@mdimjasevic mdimjasevic force-pushed the wpb-8881/effect-action-unit-tests branch from bca9dea to 184c598 Compare November 6, 2024 15:36
@mdimjasevic mdimjasevic merged commit a63c044 into develop Nov 6, 2024
@mdimjasevic mdimjasevic deleted the wpb-8881/effect-action-unit-tests branch November 6, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants