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.
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
Add setup for functional tests. Test sending events over the local variable limit. #85
Add setup for functional tests. Test sending events over the local variable limit. #85
Changes from all commits
3be6667
ed56a27
56c0fe8
e97a7a5
8d4a0d3
aa7d1be
87e0e3a
493445d
36203de
5900d93
5dcb5ff
a266ab0
d2a25db
e6727a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Question (❓): Although I understand how this test is working and what it tried to verify, I feel that something is missing here, an assertion of some kind, something that will showcase what was documented on the test case above:
The SDK will save the events to disk...
...and send them in the next flush interval.
At the end, when all events are sent...
...the SDK will delete the content of local storage file.
I mean, can we assert that the SDK is saving the events first? For example, will it save all
51
of them, or only the1
event that exceed the threshold, something else? Then, what happens during the first flush interval, what happens during the next flush interval, these kind of question. Apologies for trying to push things, I am just trying to review this as a very naive test reader that would like to understand this SDK through testing, assuming I don't have any prior knowledge or the SDK. Wdyt, too much? 🤔PS: I also naively tried to add something basic, the
assertThat(locallyStoredEvents).isNotEmpty
before thewaitFor { locallyStoredEvents.isEmpty() }
, to see if that will pass, but it failed. 🤷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 something I pivoted from in 8d4a0d3. IMO, the functional tests shouldn't check for such implementation details. The things we're interested in/testing here are:
There is only one flush, and it's tested 👍 I'll be extending this tests suite to cover more cases, like multiple flushes, but here we test "what would happen if user recorded more than 50 events quickly".
Good point, but it's only timing thing. It seems that assertions above take long enough to give SDK time to clean the file.
Try this:
and the test should fail!
WDYT?
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.
Thanks for the reply @wzieba ! 👍
👍 if you are also 👍 with that, I just wanted to question it a bit... 😊
Thanks for the clarification on that. 👍
Yea, it is hard to assert those things... 😭
FYI: The test should fail, I don't understand that, the test should success with this change, is it not? It failing was the actual problem I had as well, as I would have expected it to succeed before waiting... 🤔
PS: Apologies, I am a bit confused it seems... 🤷
Why did you remove this assertion from the patch you shared? 🤔
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.
Yes, sorry I made some confusion with the diff! Please ignore the diff in previous comment. But it's still case of timing - check this:
In this case the test will fail although, in theory, it shouldn't because after making the request, SDK will clean the local file. But, as the things happen on different threads, we don't have certainty what will happen first - assertion, or file deletion.
In case of this test - the assertions and JSON mapping take enough time, that, at least in theory, we shouldn't need to wait. Yet, relying on "JSON serialization being slow" as a determiner of test success is super flaky. Hence, the
waitFor
method.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.
Thanks for sharing a new diff, and I see, thanks for explaining again! ✅
I don't want to block this PR, feel free to merge it when you think it is ready, I just wanted to question it a bit just to get a better idea on the actual value of this functional test and how you would that be of use later on. 👍
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.
Thanks for the questions! We can always iterate over how those tests look now. For now, I think this test brings value - we're sure that saving and getting data from the local file is working fine and doesn't produce duplicates.
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.
👍 💯 🥇