Stats collector; kotlin; mcumgr test mock framework #65

bgiori · 2020-02-21T23:21:04Z

This PR is already large enough without refactoring a bit. So I will hold that to later changes when all the other mcumgr group managers are mocked out and tests added.

Adds the stats collector under the managers/meta package. This class is used to more-easily collect one or more statistics groups.

The stats collector test comes with a simple mock transport which implements a regular server behavior. This mock transport currently only implements the stats group commands, but additional handlers are planned to test other group managers.

codecov · 2020-02-24T22:58:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e123e66). Click here to learn what that means.
The diff coverage is 41.66%.

@@           Coverage Diff            @@
##             master     #65   +/-   ##
========================================
  Coverage          ?   13.1%           
  Complexity        ?      92           
========================================
  Files             ?      60           
  Lines             ?    1869           
  Branches          ?     179           
========================================
  Hits              ?     245           
  Misses            ?    1596           
  Partials          ?      28

Impacted Files	Coverage Δ	Complexity Δ
...untime/mcumgr/managers/meta/StatisticsCollector.kt	`73.91% <100%> (ø)`	`5 <0> (?)`
...ore/src/main/java/io/runtime/mcumgr/util/CBOR.java	`37.5% <39.13%> (ø)`	`5 <5> (?)`

twyatt

This is a rather large PR with big changes making it hard to give it a good critique.

The title implies that it could've been broken out into 3 different PRs:

Stats collector
kotlin
mcumgr test mock framework

mcumgr-core/build.gradle

mcumgr-core/src/main/java/io/runtime/mcumgr/managers/meta/StatisticsCollector.kt

twyatt · 2020-02-24T23:58:23Z

mcumgr-core/src/main/java/io/runtime/mcumgr/managers/meta/StatisticsCollector.kt

+    /**
+     * List the stat group names from the device and collect each which intersects with the filter.
+     */
+    fun collectAll(filter: Set<String>? = null, callback: StatCollectionCallback): Cancellable {


You pulled in the Coroutines dependency but then utilizing legacy data flow paradigms. Is it because tools that rely on this library don't support modern data flow?

The coroutines is currently used in the unit tests, I can make it a testImplemetation

The reason why the external API is not coroutines is to allow for people to use the traditional callbacks if they want (like we do...). Building coroutines support on top of this API would be very simple needed.

If the plan is to make Coroutines a first class citizen, then perhaps it would be better to use Coroutines in the library then provide an "extension" library that wraps the Coroutine versions to provide the callback implementation?

I would love to have coroutines as a first-class citizen. The issue I see is that the current architecture for mcumgr does not include it whatsoever; so any coroutines addition to the current major version 0.x.x would simply be at a wrapping-api level. The crux of coroutine support would be at the transport interface.

I think we should plan a v1.x.x with coroutines built in all the way down to the transport interface and beyond, with a callback based api-wrapper (essentially the opposite of what we would do now).

In the end, I think adding corountes would be great, just not so in this PR.

mcumgr-core/build.gradle

mcumgr-core/src/main/java/io/runtime/mcumgr/managers/meta/StatisticsCollector.kt

mcumgr-core/src/main/java/io/runtime/mcumgr/util/CBOR.java

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

bgiori · 2020-02-25T00:29:43Z

This is a rather large PR with big changes making it hard to give it a good critique.

The title implies that it could've been broken out into 3 different PRs:

Stats collector

kotlin

mcumgr test mock framework

The reason it's all in one is because each relies on another. In order to add the stats collector, we need kotlin support, and generally I would add the tests for a certain feature with the PR where that feature is added.

So if we split it out we would have a few line PR to add kotlin w/out any kotlin code in the repo, then a PR to add the stats collector, then a PR to add the stats collector tests -- Each relying on the one before.

You seemed to have gone through most of it ok.

bgiori · 2020-02-25T00:40:05Z

@twyatt Also, this is all already in the gateway SDK. I just wanted to pull it out into here for external use and as a base for additional unit tests.

Co-Authored-By: Travis Wyatt <travis@juul.com>

twyatt

Was is the middle of reviewing when this was merged, so I'll just post these comments and you can take them or leave em.

twyatt · 2020-02-28T09:48:35Z

mcumgr-core/build.gradle

@@ -26,19 +27,24 @@ android {

 dependencies {

+    // Kotlin
+    implementation "androidx.core:core-ktx:1.2.0"
+    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"


Suggested change

implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"

implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"

twyatt · 2020-02-28T09:55:23Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+    @Test
+    fun `parse image without protected tlvs success`() {
+        val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!
+            ?: throw IllegalStateException("input stream is null")


Suggested change

?: throw IllegalStateException("input stream is null")

twyatt · 2020-02-28T09:56:08Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+        val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")
+            ?: throw IllegalStateException("input stream is null")


nitpick

Suggested change

val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")

?: throw IllegalStateException("input stream is null")

val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")!!

twyatt · 2020-02-28T10:03:42Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+        val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!
+            ?: throw IllegalStateException("input stream is null")
+        val imageData = toByteArray(inputStream)


nitpick: could make these more readable with a quick refactor.

fun String.readByteArray() = McuMgrImageTest::class.java.classLoader!! .getResourceAsStream(this) .toByteArray()

Suggested change

val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!

?: throw IllegalStateException("input stream is null")

val imageData = toByteArray(inputStream)

val imageData = "slinky-no-prot-tlv.img".readByteArray()

twyatt · 2020-02-28T10:06:06Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+    private fun toByteArray(inputStream: InputStream): ByteArray {
+        val os = ByteArrayOutputStream()
+        val buffer = ByteArray(1024)
+        var len: Int
+        while (inputStream.read(buffer).also { len = it } != -1) {


nitpick

Suggested change

private fun toByteArray(inputStream: InputStream): ByteArray {

val os = ByteArrayOutputStream()

val buffer = ByteArray(1024)

var len: Int

while (inputStream.read(buffer).also { len = it } != -1) {

private fun InputStream.toByteArray(): ByteArray {

val os = ByteArrayOutputStream()

val buffer = ByteArray(1024)

var len: Int

while (read(buffer).also { len = it } != -1) {

twyatt · 2020-02-28T10:09:20Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+package io.runtime.mcumgr
+
+import io.runtime.mcumgr.image.McuMgrImage
+import org.junit.Test
+import java.io.ByteArrayOutputStream
+import java.io.InputStream
+
+class McuMgrImageTest {
+
+    @Test
+    fun `parse image without protected tlvs success`() {
+        val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!
+            ?: throw IllegalStateException("input stream is null")
+        val imageData = toByteArray(inputStream)
+        McuMgrImage.fromBytes(imageData)
+    }
+
+    @Test
+    fun `parse image with protected tlvs success`() {
+        val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")
+            ?: throw IllegalStateException("input stream is null")
+        val imageData = toByteArray(inputStream)
+        McuMgrImage.fromBytes(imageData)
+    }
+
+    private fun toByteArray(inputStream: InputStream): ByteArray {
+        val os = ByteArrayOutputStream()
+        val buffer = ByteArray(1024)
+        var len: Int
+        while (inputStream.read(buffer).also { len = it } != -1) {
+            os.write(buffer, 0, len)
+        }
+        return os.toByteArray()
+    }
+}


For readability, the combination of the above suggestions:

class McuMgrImageTest { @Test fun `parse image without protected tlvs success`() { McuMgrImage.fromBytes("slinky-no-prot-tlv.img".readByteArray()) } @Test fun `parse image with protected tlvs success`() { McuMgrImage.fromBytes("slinky-prot-tlv.img".readByteArray()) } } private fun String.readByteArray() = McuMgrImageTest::class.java.classLoader!! .getResourceAsStream(this) .toByteArray() private fun InputStream.toByteArray(): ByteArray { val os = ByteArrayOutputStream() val buffer = ByteArray(1024) var len: Int while (read(buffer).also { len = it } != -1) { os.write(buffer, 0, len) } return os.toByteArray() }

twyatt · 2020-02-28T10:09:48Z

mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt

+package io.runtime.mcumgr
+
+import io.runtime.mcumgr.image.McuMgrImage
+import org.junit.Test


Suggested change

import org.junit.Test

import kotlin.test.Test

bgiori · 2020-02-28T21:07:25Z

Was is the middle of reviewing when this was merged, so I'll just post these comments and you can take them or leave em.

Ah sorry! Thanks! I will be making changes to build out the mock transport to support other groups and will take these comments into consideration.

bgiori added 6 commits February 21, 2020 15:18

stats collector; kotlin; mcumgr test mock framework

bcd44e6

update gradle and other dependencies to fix build

774d187

update jackson, retain old CBOR api

57b63fe

add handlers pkg

1533e8d

make kotlin test implementations

6cbfbbf

whoops

e123e66

bgiori requested review from davidtaylor-juul, Chris-Corea, ebabel and twyatt February 24, 2020 18:28

bgiori added 2 commits February 24, 2020 13:45

Merge branch 'master' into brian/stats-collector

00d2f89

add line removed by merge

749749b

twyatt reviewed Feb 25, 2020

View reviewed changes

bgiori and others added 6 commits February 24, 2020 16:42

coroutines testImplementation

9507204

revert CBOR, use mvp

758c302

Apply suggestions from code review

3256083

Co-Authored-By: Travis Wyatt <travis@juul.com>

fix build

195fa2a

remove extensions fix package def

110fb91

remove unecessary repo

5c9c578

bgiori requested a review from twyatt February 25, 2020 23:53

davidtaylor-juul approved these changes Feb 26, 2020

View reviewed changes

ebabel approved these changes Feb 27, 2020

View reviewed changes

bgiori merged commit e2e062b into master Feb 28, 2020

twyatt reviewed Feb 28, 2020

View reviewed changes

philips77 pushed a commit that referenced this pull request Apr 3, 2020

Stats collector; kotlin; mcumgr test mock framework (#65)

6c2f9e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats collector; kotlin; mcumgr test mock framework #65

Stats collector; kotlin; mcumgr test mock framework #65

bgiori commented Feb 21, 2020 •

edited

Loading

codecov bot commented Feb 24, 2020 •

edited

Loading

twyatt left a comment

twyatt Feb 24, 2020

bgiori Feb 25, 2020

bgiori Feb 25, 2020

twyatt Feb 25, 2020 •

edited

Loading

bgiori Feb 25, 2020

bgiori commented Feb 25, 2020

bgiori commented Feb 25, 2020

twyatt left a comment

twyatt Feb 28, 2020

twyatt Feb 28, 2020

twyatt Feb 28, 2020

twyatt Feb 28, 2020

twyatt Feb 28, 2020

twyatt Feb 28, 2020

twyatt Feb 28, 2020

bgiori commented Feb 28, 2020

	implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
	implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"

		val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")
		?: throw IllegalStateException("input stream is null")

Stats collector; kotlin; mcumgr test mock framework #65

Stats collector; kotlin; mcumgr test mock framework #65

Conversation

bgiori commented Feb 21, 2020 • edited Loading

codecov bot commented Feb 24, 2020 • edited Loading

Codecov Report

twyatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgiori commented Feb 25, 2020

bgiori commented Feb 25, 2020

twyatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgiori commented Feb 28, 2020

bgiori commented Feb 21, 2020 •

edited

Loading

codecov bot commented Feb 24, 2020 •

edited

Loading

twyatt Feb 25, 2020 •

edited

Loading