Payload releasing #108
Payload releasing #108
Conversation
use leak detection in all core tests + transport test for local connection fix some releasing issues, still WIP
remove suppress for some functions
| } | ||
| } | ||
|
|
||
| interface TestWithLeakCheck { |
There was a problem hiding this comment.
I came here to suggest this. This is great!
|
One bit of critical feedback. I feel like this should be multiple PRs. I'm not suggesting this to ask you to change it (don't! just land it), but that given the N:1 reviewer ratio, if you made the internal changes in a single PR. Added pooling. Added the leak checking. |
| } | ||
|
|
||
| suspend fun FlowTurbine<*>.expectNoEventsIn(timeMillis: Long) { | ||
| delay(timeMillis) |
There was a problem hiding this comment.
Do we have a concept of mocked time in ktor, rsocket-kotlin or otherwise?
There was a problem hiding this comment.
there is mocked time in kotlinx-coroutines-debug with test dispatcher. But I'm not sure, that it will work as expected, as in most places, delay is used to handle asynchronous cancellation of jobs, channels. But that can be of course investigated
There was a problem hiding this comment.
It will affect you more than me. But yeah, often the time based tests are the least deterministic, and overtime cause the most wasted developer & CI wallclock time. Getting something good in place can literally mean the difference between 1-2 second tests and 1-2 minutes.
yschimke
left a comment
yschimke
left a comment
There was a problem hiding this comment.
Nice!
It's a tough review given the size, formatting changes, internal/testing changes and the meat of the change.
I'd love to test this with the other PR at the same time from snapshots.
README.md
Outdated
| The `master` branch is now dedicated to development of multiplatform rsocket-kotlin. | ||
| For now only snapshots are available via [oss.jfrog.org](oss.jfrog.org) (OJO). | ||
| The `master` branch is now dedicated to development of multiplatform rsocket-kotlin. For now only snapshots are available | ||
| via [oss.jfrog.org](oss.jfrog.org) (OJO). |
There was a problem hiding this comment.
Maybe link to https://oss.jfrog.org/artifactory/oss-snapshot-local/io/rsocket/kotlin/ with an example for using a snapshot.
|
@yschimke yes, I understand, that it would be better to split in in 2: leak detection and |
TestConnectionto useturbineinstead of handmade checks