View interaction wrapper

[dkostyrev]

Introduce ViewInteractionWrapper and ViewInteractionWrapperFactory to be able to proxy and intercept calls to Espresso ViewInteraction made by Kakao.

Possible use cases:

• Logging
• Implementing retry wrappers

[CLAassistant]

All committers have signed the CLA.

[Vacxe]

@dkostyrev Thanks for your contribution! We will review this implementation as soon as possible. Also I think that we could make this implementation more flexible.

[Unlimity]

Hey there! Thanks for your contribution! However, I don't think that your approach matches the style of the library. As I undestood, main goal here is to give users access to withFailureHandler. If I'm not mistaken, view property is accessible in every KView class out there, so there's actually no need to wrap ViewInteraction to achieve such behaviour. So to fully understand what are you trying to achieve here, please provide a couple of use examples. The idea with retries is good though, and can be implemented in functional DSL way, something like:

view {
  retryOnError(3, 1000L) {
    click()
  }
}

[Unlimity]

Ok, so I went through the code again and I got your point. But still it looks not so pretty as it could be. I would suggest making something like RxJava hooks. So basically, we have our own internal wrapper with hooks and we expose this via some additional interface. Something like that:

view {
  onAssertion { assertion ->
    Log.d("TEST", assertion)
  }

  onAction { action ->
    Log.d("TEST", action)
  }

  onError { throwable ->
    Log.e("TEST", throwable)
  }
}

What do you think?

[dkostyrev]

Hello. The main idea behind this is to hide retries inside Kakao and do not clutter test code. I agree that it can be improved, though. I am also thinking about keeping original interfaces and wrap every perform and check code at call site.

[Unlimity]

I looked in your last commit and it actually not what I meant. I think wrapping calls on every function is not the best solution. I still would suggest have ViewInteraction wrapped, but with hook-able wrapper and also introduce hook functions on KView classes.

[dkostyrev]

Wrapping calls to ViewInteraction will allow to keep original interfaces, I agree that it is less convenient than wrapped ViewInteraction. What about keeping view: ViewInteraction property accessible but deprecated?

[dkostyrev]

I've reverted changes with wrapping calls and added KBaseView hooks. You can see examples here.

[Unlimity]

Hey there! Looks better, but I still think there is a lot to improve.

1) Let's not do anything with DataInteraction at this point. It should be done in separate PR. 2) KakaoInterceptors object is not necessary, hooks should be stored in corresponding KView's wrapper. 3) No need to have dedicated interface for each hook, we have function types in Kotlin. Also, there is no need to pass instance of ViewInteraction or DataInteraction to corresponding hook, since it will be declared on a specific KView instances during the test. Also, there is no need to return anything from these hooks. 4) Naming. Since we have *Actions and *Assertions interfaces used across the library, to keep the consistency we need to call hooking functions as onAction and onAssertion. 5) No need to have nullable vars holding hooks, you can make non-nullable hook with empty function as default value thus avoiding any checks and invoke every wrapped function as you already have a hook. Boolean?.orFalse will be redundant in this case.

That's the brief comments that I have for now. Please keep everything as simple as possible. Cheers!

[Unlimity]

Also, I don't think we need to wrap on every wrapped function call. You can just store var of wrapped object and update it inside. Thus returning the same instance of wrapper on each chain call.

[dkostyrev]

1. DataInteraction should be wrapped as well, otherwise it will not work properly with KAdapterItems. I don't think that it should be done in separate PR.
2. The whole point of this PR was to introduce global interaction hooks, keeping tests clutter free. I agree that there should be a way to override them locally.

3. there is no need to pass instance of ViewInteraction or DataInteraction

I don't get how it should work then. Hooks allow to perform actual perform or check call on ViewInteraction outside of the ViewInteractionWrapper. Hook should return true when it actually called perform or check on ViewInteraction and false otherwise to prevent ViewInteractionWrapper from calling it.

5. In case with non-nullable hooks - how user will remove it?

[Unlimity]

1. Maybe. Still discussable, I think we need someone else's opinion on this.
2. Global and local level hooks are completely different things. I was looking at the code from local hooks perspective. I don't think that global hooks will be widely used for anything except logging. And logging can be implemented at the library level.
3. Why do we need to perform something with these instances in a hook? I can think of only one scenario for this: retry strategies. And this can be done at the library level as well. On the local hook level though, it is completely not needed, since developer knows exactly which view was hooked. Generally I don't like the idea of exposing Espresso related things through the library since it's general purpose is to hide it behind fluent and consistent DSL.
4. In reset function you just revert the value back to the default empty one.

Cheers!

[dkostyrev]

2. I was thinking about implementing retry strategies with global hooks.

3. Can you elaborate more on

this can be done at the library level

?

I don't like the idea of exposing Espresso related things through the library

But it already exposes ViewInteraction in *Actions and *Assertions interfaces.

[Unlimity]

I think retry strategies should apply locally on specific code blocks, thus it would be much more nicer to have syntax which I proposed in the beginning of this discussion. Doing this stuff at library level is essential so that we will not bloat it's users with details of how it's implemented. Take a look at Glide, for example. Logging is implemented at the library level. You just set the verbosity level. Same goes for retry strategies. RxJava has it as operator, OkHttp does it during client builder. There are a lot of ways how we can implement it internally. I still would prefer to have hooks just like the Rx* has it, simple callbacks with no access to control the flow. Regarding exposed view property, interfaces in Kotlin cannot have non-public properties. I'm also considering to switch to abstract classes to hide this access. And even though it is exposed right now, you don't need to access it to have everything working as it was designed. Explicitly exposing espresso's classes for "special" cases, which Kakao can't handle, is the same as saying "Hey, we covered 90% of the stuff for you, but these 10% you need to do yourself". I'm against that.

[cdsap]

hi @dkostyrev, first of all, thank you very much for your contribution and taking time to check and add value to the project. Looks like we are stuck in this conversation but I would like to invite you to open an issue where the community and us can discuss the improvement exposed in your PR. These changes can be different from the view of the project within our roadmap, but nothing is static and that's why we have the issues to discuss feature requests or new ideas like yours.

Do you mind to close this PR and create an Issue where we can discuss it?

Thank you very much

[dkostyrev]

Sure, I will open the issue to discuss it first