fix KotlinConstructorBuilder on Android SDK below 26 #2881
Conversation
| super.onCreate(savedInstanceState) | ||
| setContentView(R.layout.activity_main) | ||
| val someText = checkNotNull(jsonAdapter.fromJson("""{"value": "Hi"}""")) | ||
| findViewById<TextView>(R.id.text_view).text = someText.value_ |
There was a problem hiding this comment.
Since we're fixing a runtime problem, we need this code to run on CI to be useful. We thus need a test that execute this path.
Right now, without your fix, this would not throw either since CI isn't launching the app.
There was a problem hiding this comment.
hmmmm does that mean we need androidTests with an emulator...? hmmm
There was a problem hiding this comment.
Yup - I would love to write a test but I am both unfamiliar with the CI setup, and I don't think the current build configuration supports any of the ways I can think of to add a regression test for this, so your input would be helpful on what you think would be best. I don't want to introduce something unmaintainable just for this one test.
Right now I can think of a few ways this might be done, all of which are pretty icky in the context of what exists in the project already:
- Run unit tests on an older JDK: don't think the project has any support for this
- Use a mocking framework (e.g. MockK or Mockito): one is not already used in the project
- Use Robolectric: I don't even know if it would pick this up, but it is not part of this project
- Run instrumentation tests: Could use Gradle managed devices, or Firebase Test Lab. For the former, would need bare metal machines (which are probably already being used in the form of Mac agents?)
Let me know what you think.
There was a problem hiding this comment.
I think I would copy/pasta as much as we need from Retrofit
https://github.com/square/retrofit/blob/trunk/.github/workflows/build.yml#L30-L64
If that sounds doable to you, please! otherwise I can tackle that when I have the time
There was a problem hiding this comment.
Just what I need, thanks! Will do.
By the way, it looks like com.squareup.wire.GrpcClientTest > streamingRequestFailureWithUnexpectedHttpCode might be flaky
There was a problem hiding this comment.
Added - fwiw I wasn't able to use assertj because it depends on other JDK APIs that can't be desugared
| @@ -108,7 +108,6 @@ internal class KotlinConstructorBuilder<M : Message<M, B>, B : Message.Builder<M | |||
| .sortedBy { it.wireField.schemaIndex } | |||
|
|
|||
| private class ProtoField( | |||
| // TODO(Benoit) Delete if unused? | |||
There was a problem hiding this comment.
This was waiting for you it seems
|
Thanks a lot Damian, chef-kiss PR. |
|
Thanks for merging! Are there any plans to release another stable version ( |
fix KotlinConstructorBuilder on Android SDK below 26
|
|
|
Amazing, thank you! Will do 🙇 |
Fixes #2876. I also added a sample app that can run on API 24+ per @oldergod's request, and verified that it no longer crashes.