Fix some setup/teardown in tests to take care of isolation issues. #169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evan-masseau
commented
May 22, 2024
| @@ -94,6 +95,10 @@ internal class KlaviyoTest : BaseTest() { | |||
| @After | |||
| override fun cleanup() { | |||
| Registry.get<State>().reset() | |||
| Registry.unregister<Config>() | |||
There was a problem hiding this comment.
This is the key part for the failing test on Ajay's PR.
There was a problem hiding this comment.
Can you tell me more on how this was causing the issue. I would have thought it's related to state.
There was a problem hiding this comment.
Oh sorry, it is state, commented on the wrong line
There was a problem hiding this comment.
The issue was that in the setup of this test class we call Klaviyo.initialize and within initialize we register config, state, state side effects, so in teardown we have to unregister those.
My idea for a bigger fix would be to isolate registry a bit more so that each test starts with a clean registry and you don't have to unregister individually like this in teardowns.
evan-masseau
commented
May 22, 2024
|
|
||
| @After | ||
| fun cleanup() { | ||
| Registry.unregister<Log>() |
There was a problem hiding this comment.
This was a similar leak, just wasn't causing any failed tests (yet)
ajaysubra
added a commit
that referenced
this pull request
May 22, 2024
…t in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com>
dan-peluso
added a commit
that referenced
this pull request
Oct 23, 2024
* Separation of API, State, and Side Effects (#160) * Separation of API, State and Side Effects. Largely left tests alone except for adding a couple and adjusting setup to accommodate changes. * Use consistent nullability pattern so we aren't converting between empty string and null with identifiers. Added proper deserializing of the profile attributes store property * There is indeed a better way to do that * Renamed some of the new classes and added new tests for them * Doc blocks, minor cleanup, naming conventions * Fix nullability test Don't need immutable profile to expose attributes, but would be good to expose single attribute getter * First round of PR comments * missed another arg label --------- Co-authored-by: Evan Masseau <> * Set an alpha version number on this branch in case it gets pulled by a snapshot jitpack build * Remove deprecated lifecycle property (#156) Co-authored-by: Evan Masseau <> * Transition profile identifier keys to internal visibility (#155) Co-authored-by: Evan Masseau <> * Add Unregister Push Token Request (#162) * Publish old value of a state property (#164) Co-authored-by: Evan Masseau <> * Rename these properties so it is clear what true and false mean. Fix polarity of how we check background data (#166) Co-authored-by: Evan Masseau <> * Added tests for publishing of the old value (#165) * Added test coverage for publishing old value with change callback * Broadcast on reset, and tests * Reset as a cleanup step, not a setup step. --------- Co-authored-by: Evan Masseau <> * Trigger Unregister on API Key Change (#163) * Using token endpoint for profile requests when a push token is present in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com> * API Header Fix: Move attempt count increment to before we write request headers into the url connection. (#170) Co-authored-by: Evan Masseau <> * setProfileAttribute should accept any Serializable value not just String (#179) * changing profile attribute to accept serializable * updating for 3.0 release * logging incorrectly typed profile attribute * Concurrent network observer fix (#180) * using synchronized list for observers * adding concurreny safe structure to all observer instances * Refresh versions syntax for dependencies * removing concurrent suffix --------- Co-authored-by: Evan Masseau <> * Retry on 503 (#181) * Sending 503 through retry logic * removing version.properties unecessary changes * fixing http import * CHNL-6996 Proguard docs and consumer rules (#184) * Adding consumer rules and updating docs * progaurd changes * Introduce a de-dupe mechanism for push notifications (#177) * Introduce tag support so we have the option to de-dupe push notifications similar to how stock FCM sdk can. * adding constant id and null-checked notification tag --------- Co-authored-by: Evan Masseau <> Co-authored-by: Daniel Peluso <daniel.peluso@klaviyo.com> * CHNL-3990 Remove identifiers from state if format issues (#186) * Remove identifiers from state if API reports format errors * pr comments * removing extraneous decoder test * Exposing klaviyo SDK name and version privately (#185) * added klaviyo sdk name and version * Update sdk/analytics/src/main/java/com/klaviyo/analytics/DeviceProperties.kt Co-authored-by: dan-peluso <daniel.peluso@klaviyo.com> * fixed some formatting * updated the SDK name to be non lazy --------- Co-authored-by: dan-peluso <daniel.peluso@klaviyo.com> * Refactor singleton service initializers (#172) * Convert SystemClock to class * These shouldn't have been mocked in this test class, its masking a bug. * Remove unnecessary, slightly risky, object initializers in core. * Restore system clock to object -- I take it back, we don't have to get rid of objects, just have to be more particular about using object initializers. * KLog can be a singleton object, no risk there. * This simple fix would take care of the issue, but i still don't love KlaviyoApiClient being a singleton object that can hit an unrecoverable exception in its initializer... * Found a balanced solution for KlaviyoApiClient -- converting this to a class was complicated by the inner class, and besides it works well as a singleton. The initializer was the real problem, so I extract the listeners from the initializer and created a startService method. This is safe to run multiple times if the SDK is re-initialized. * Prefer this way: we don't need to wrap any of the code in this method body except the methods that are themselves protected anyway * Forgot to add back starting the service in these tests' setup * Naming convention things * First shot at a pre-init buffer for failed operations. Needs tests * Added test of in-memory slate * I don't like import * * Added comments, logging, and wrapped the retry operations in safeCall * Trivial: move error logging out of exception class. Now that we wrap all SDK functions in safeCall, it is more straightforward to log from there. * Allow ONLY handlePush to be buffered * Remove unused test * keep 3.0 version increment * don't add this back * never trust github * Update versions.properties --------- Co-authored-by: Evan Masseau <> * Automatically check push permission status on app resume (#188) * Protect against double subscriptions * readme and migration guide updates --------- Co-authored-by: Evan Masseau <> * Move API revision to core config property pulled from build property (#190) Co-authored-by: Evan Masseau <> * [CHNL-12521] Looking for react-native strings to determine sdk name and version for config (#191) * resource reading maven attmept * working resource share attempt * update log level * fixing unit tests and working resource sharing * pr comments * fixing config * replace bump version task with xml-based task * fixing unit test * fixing file pathing for composite builds * fixing bump version task * removing build config field from build.gradle * using readXml for default config * removing unused versionFor import * removing capability change broadcast (#192) * renaming removals to 'breaking changes' (#193) * bumping version --------- Co-authored-by: Evan Masseau <> Co-authored-by: Kenny Tsui <63658871+kennyklaviyo@users.noreply.github.com> Co-authored-by: Ajay Subramanya <118314354+ajaysubra@users.noreply.github.com> Co-authored-by: dan-peluso <daniel.peluso@klaviyo.com>
dan-peluso
pushed a commit
that referenced
this pull request
Jan 14, 2025
…t in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com>
dan-peluso
pushed a commit
that referenced
this pull request
Feb 18, 2025
…t in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com>
dan-peluso
pushed a commit
that referenced
this pull request
Feb 18, 2025
…t in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To fix an issue introduced when I made state into a registered service:
KlaviyoTestis leaking some registered services.I have an idea for a better long-term fix that'd change
Registrya little bit to make it easier to isolate tests.