Fix android crash on startup #1954
Conversation
anjmao
commented
Mar 30, 2020
anjmao
commented
- Fix for Android app crash with nil pointer dereference in AccountantPromiseSettler #1952
- Add e2e test for mobile entry point.
- Remove di_mobile.go and use flag to determine if specific logic is needed when consumer is mobile.
anjmao
requested review from
soffokl,
tadaskay,
vkuznecovas,
Waldz and
zolia
as code owners
anjmao
force-pushed
the
mobile-e2e
branch
3 times, most recently
from
22d1e5b
to
5d94708
Compare
anjmao
force-pushed
the
mobile-e2e
branch
3 times, most recently
from
33ffcf7
to
b5ad87b
Compare
| @@ -62,6 +61,10 @@ import ( | |||
|
|
|||
| // bootstrapServices loads all the components required for running services | |||
| func (di *Dependencies) bootstrapServices(nodeOptions node.Options, servicesOptions config.ServicesOptions) error { | |||
| if nodeOptions.MobileConsumer { | |||
There was a problem hiding this comment.
What was wrong with build flags approach?
Because now file is di_desktop.go but full of conditional logic, which would be nice to avoid
There was a problem hiding this comment.
- I can't build mobile entry point using
go build ./mobile/mysteriumsince di_mobile will not be included but di_mobile doesn't contain any OS specific logic. - There are multiple checks already with feature flags, example https://github.com/mysteriumnetwork/node/blob/master/cmd/di_desktop.go#L342.
- I can't write e2e test properly. If I add GOOS=android it will require specific C++, but for now I just want to write e2e test which tests that mobile entry point doesn't crash on startup, so if connect is not working at least use can sent feedback.
There was a problem hiding this comment.
I too am for conditional compiling, if possible.
There was a problem hiding this comment.
Agree with @Waldz here. We should not increase our codebase with platform checks inside if we can build accordingly.
There was a problem hiding this comment.
80% of current mobile entrypoint API can be tested without any android specific build tag and most importantly it tests for possible crashes when some dependencies are not added to mobile . If you still want conditional compiling I can close this PR and add a separate task since it will require 3-4 days at least to make it work. @zolia
There was a problem hiding this comment.
So it's just for the test, but not released artefact, ASFAIK.
Is there a way to force a android tag in this test particularly. Maybe adding custom flag like "e2e" is what you need?
|
@Waldz @zolia @vkuznecovas I merge it as is for now. If you have ideas how to add android with build tags to our e2e tests fast and cheap without waisting time let me know :) |
