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

Added unit tests for subscriptions #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emarty-maze
Copy link
Contributor

Added sms and email opt-in e2e tests for sitegen
Moved global variables to module
Refactored order confirmation unit test
Setup mock for KlaviyoSubscribeProfilesService
Refactor Site and LocalServiceRegistry Mocks
Completed subscriber tests

Manual Testing Steps

  1. Using node 18.16.x, cd into test dir
  2. Run npm install
  3. Run npm run test:unit

Pre-Submission Checklist:

  • You've updated the CHANGELOG following the steps here

NOTE: Please use the Changelogger cli tool to manage versioned file upgrades.

I followed cases from the following google doc: https://docs.google.com/document/d/1e4p75l_w0P6QfTqWja6sW8rfHGtNbvE5NWXxYszYne4/edit

@emarty-maze emarty-maze requested a review from a team as a code owner July 19, 2023 00:51
@klaviyoit klaviyoit requested a review from smoucka July 19, 2023 00:51
Added sms and email opt-in e2e tests for sitegen
Moved global variables to module
Added unit test for subscriptions
Refactored order confirmation unit test
Setup mock for KlaviyoSubscribeProfilesService
Refactor Site and LocalServiceRegistry Mocks
Completed subscriber tests
@emarty-maze emarty-maze force-pushed the feature/KSCU-78_subscriber_tests branch from 3f5e372 to c0ef967 Compare July 19, 2023 04:30
@smoucka smoucka requested review from cykolln and removed request for smoucka July 19, 2023 13:25
Copy link
Contributor

@cykolln cykolln left a comment

Choose a reason for hiding this comment

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

@emarty-maze If I'm reading these correctly, these tests evaluate the klaviyo API response when requests are made but don't evaluate the actual behavior of the cartridge code. The code has different behavior in terms of what requests it makes and how many requests are made depending on the phone_number and email that are submitted. I'd like to see these conditionals in the subscribeUser function covered. For example the case for email consent if we receive a 400 with the response indicating an invalid phone number that we should retry the consent request without the phone number. There should be a test verifying that this request is made twice given the 400 API response.

Responses from the API can be mocked but we should be testing that requests are made as expected with the correct arguments and that logging occurs as expected where relevant. Let me know if I need to clarify this, happy to jump on a quick call to discuss!

@emarty-maze
Copy link
Contributor Author

emarty-maze commented Jul 24, 2023 via email

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