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

Tests for incoming messages don't check that failure is due to the right reason #49

Closed
gnarea opened this issue Mar 9, 2021 · 3 comments · Fixed by #230
Closed

Tests for incoming messages don't check that failure is due to the right reason #49

gnarea opened this issue Mar 9, 2021 · 3 comments · Fixed by #230
Assignees
Labels
bug Something isn't working released

Comments

@gnarea
Copy link
Member

gnarea commented Mar 9, 2021

I think we should improve the tests for the following because it's far too easy to write tests that fail for the wrong reason:

https://github.com/relaycorp/relaynet-endpoint-android/blob/95a47a627f31fc4d5715b029ea7e342839b7e6d3/lib/src/main/java/tech/relaycorp/relaydroid/messaging/ReceiveMessages.kt#L68-L91

Maybe we should inspect the logs?

For example, the current implementation has a bug which the tests aren't uncovering: There are two catch blocks for the same exception:

https://github.com/relaycorp/relaynet-endpoint-android/blob/95a47a627f31fc4d5715b029ea7e342839b7e6d3/lib/src/main/java/tech/relaycorp/relaydroid/messaging/ReceiveMessages.kt#L82-L86

I came across this whilst implementing #48, because my tests were passing before I changed the unit under test.

@gnarea gnarea added the bug Something isn't working label Mar 9, 2021
@Filmaluco
Copy link
Contributor

Hi @gnarea, another task that landed on my lap :)
So before I start I wanted to check with you, if I understand what you want ... solutions to better test the reasons collectParcels might/is failing... correct?


Assuming that I understood correctly, you started suggesting

  • "log" exceptions and test said log data

I would like to give a couple of other suggestions that I think might work as well, might be simpler to implement and more useful in the future:

  • Delegate and move the collectParcles to another class and make that class internal, having better access to test potential scenarios.
  • Optional callback system that we can use to intercept exceptions

what do you think @gnarea?

@gnarea
Copy link
Member Author

gnarea commented May 5, 2022

So before I start I wanted to check with you, if I understand what you want ... solutions to better test the reasons collectParcels might/is failing... correct?

Yes. The current assertions are insufficient to tell the tests are passing for the right reasons.

BTW, I recently integrated log captor, a library to capture logs in tests.

@kodiakhq kodiakhq bot closed this as completed in #230 May 18, 2022
kodiakhq bot pushed a commit that referenced this issue May 18, 2022
Fixes #49

@gnarea Notice the failing test. Feels weird that deleting the ThirdPartyEndpoint is not enough, since it triggers another error. Are we forgetting to delete something inside the `delete()` method?
@github-actions
Copy link

🎉 This issue has been resolved in version 1.12.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants