fix: stack-overflow caused by BQ recursion

[Shahroz16]

closes: https://github.com/customerio/customerio-android/issues/248

[github-actions[bot]]

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.0.1. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format: ``` : short description of change being made ``` **If your pull request [introduces breaking changes](https://web.archive.org/web/20220725195319/https://nordicapis.com/what-are-breaking-changes-and-how-do-you-avoid-them/)** to the code, use this format: ``` !: short description of breaking change ``` where `` is one of the following: - `feat:` - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project. - `fix:` - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project. - `docs:` - This pull request is making documentation changes, only. - `refactor:` - A change was made that doesn't fix a bug or add a feature. - `test:` - Adds missing tests or fixes broken tests. - `style:` - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc) - `perf:` - Changes improve performance of the code. - `build:` - Changes to the build system (maven, npm, gulp, etc) - `ci:` - Changes to the CI build system (Travis, GitHub Actions, Circle, etc) - `chore:` - Other changes to project that don't modify source code or test files. - `revert:` - Reverts a previous commit that was made. ### Examples: ``` feat: edit profile photo refactor!: remove deprecated v1 endpoints build: update npm dependencies style: run formatter ``` Need more examples? Want to learn more about this format? [Check out the official docs](https://www.conventionalcommits.org/). **Note:** If your pull request does multiple things such as adding a feature _and_ makes changes to the CI server _and_ fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

[github-actions[bot]]

# Sample app builds 📱 Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. --- * java_layout: `shahroz/fix-bq-outofstack (1694540814)` * kotlin_compose: `shahroz/fix-bq-outofstack (1694540809)`

[codecov[bot]]

Codecov Report

Merging #251 (80abdd7) into main (2d32664) will increase coverage by 0.03%. Report is 2 commits behind head on main. The diff coverage is 97.56%.

@@             Coverage Diff              @@
##               main     #251      +/-   ##
============================================
+ Coverage     50.80%   50.84%   +0.03%     
  Complexity      249      249              
============================================
  Files           108      108              
  Lines          2781     2779       -2     
  Branches        364      361       -3     
============================================
  Hits           1413     1413              
+ Misses         1250     1249       -1     
+ Partials        118      117       -1     

Files Changed	Coverage Δ
...main/java/io/customer/sdk/queue/QueueRunRequest.kt	98.00% <97.56%> (+7.61%)	:arrow_up:

... and 1 file with indirect coverage changes

[github-actions[bot]]

Build available to test Version: shahroz-fix-bq-outofstack-SNAPSHOT Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

[Shahroz16]

@levibostian Thanks for looking into it,

GitHub annotations highlights a couple places where there is code that does not get executed by tests (meaning possible missing tests)

The only highlight seems towards log statements it seems.

This is a critical part of the code base, execution of the BQ. I strongly suggest that more tests are written for this run function to cover all the possible edge cases.

Can you explain what specific tests are you looking for? I have added some more but asking because we didn't change any logic except move from recursion to iterative approach. So, what gave you confidence in the previous implementation with the current test suite, but now makes you feel less confident?

[levibostian]

@Shahroz16 Great questions.

The latest test that you added for when a task cannot be found in storage was 1 missing test case. That's a great test case to add, thank you.

Github annotations is telling me that there is 1 more missing branch for this conditional statement. Adding a test for that scenario would be great.

You do have a good point in that this is a refactor so existing tests should be all that we need. In my original comment I could have explained better that even though this is a refactor, I don't want to ship this code until the test suite is improved upon. If there are missing tests, I wanted to make sure that we add tests to the existing suite to feel more confident in the code.

I recently found a missing test case in this exact same code block in the iOS SDK. When I added the missing test case, I actually found a bug. This makes me want to double check our test suite around this code in the Android SDK to make sure we trust it. I believe this PR is a good opportunity to do that.

I reviewed the current test suite and it looks good to me except:

• this conditional statement missing a test
• Non-blocker for this PR, QueueIntegrationTests - could we add a test to reproduce this stackoverflow error? Then make sure that this refactor fixes the issue?

[Shahroz16]

@levibostian added more tests.

Regarding,

could we add a test to reproduce this stackoverflow error? Then make sure that this refactor fixes the issue?

I did try, but wasn't able to do it because it depends on multiple factors like heap size and limiting it isn't that straight forward with unit tests.

[levibostian]

Tests are great! Thanks!

Oh, I just realized there is 1 more conversation to have resolved. I do consider this a blocker for merging this PR.

[Shahroz16]

@levibostian just noticed my comment to it was never posted so did it again, it says 2 days ago for me that's why I originally added it. Just making sure, you can see it now.