Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Library to AndroidX #89

Merged
merged 8 commits into from Jan 13, 2019
Merged

Migrate Library to AndroidX #89

merged 8 commits into from Jan 13, 2019

Conversation

cavega
Copy link
Contributor

@cavega cavega commented Dec 21, 2018

@leandroBorgesFerreira Submitting this PR in case you want were planning on migrating the library to the new AndroidX package system. For now I'll use my fork.

  • Migrate all android.support libraries to the latest AndroidX equivalents.

  • Added test runner dependency for running Espresso tests

  • Created initial set of tests for CircularProgressButton

Copy link
Contributor

@wokkaflokka wokkaflokka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To increase the SDK version, you'll also need to update the travis CI build settings, located here: https://github.com/leandroBorgesFerreira/LoadingButtonAndroid/blob/master/.travis.yml

Changes look mostly correct to me. Would be nice to see a test.

app/build.gradle Outdated
implementation 'com.android.support.constraint:constraint-layout:1.1.0'
implementation 'com.android.support:support-v4:27.1.1'
implementation 'androidx.constraintlayout:constraintlayout:1.1.3'
implementation 'androidx.legacy:legacy-support-v4:1.0.0'
implementation fileTree(dir: 'libs', include: ['*.jar'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually pulling in a local dependency or could it be removed?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think those guys can be removed. They are needed in the sample module

build.gradle Outdated
@@ -24,6 +24,7 @@ allprojects {
repositories {
jcenter()
maven { url 'https://maven.google.com' }
google()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also add mavenLocal()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of that said, I'm comfortable with whatever @leandroBorgesFerreira is comfortable with.

@@ -1,15 +1,15 @@
// Top-level build file where you can add configuration options common to all sub-projects/modules.

buildscript {
ext.kotlin_version = '1.2.40'
ext.kotlin_version = '1.3.11'
repositories {
jcenter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mavenLocal()?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why mavenLocal()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly preferential as the gradle build system already has a dependency caching mechanism that achieves a similar effect to mavenLocal. The main purpose of declaring mavenLocal as a repository is to enable gradle to use dependencies already downloaded on a given machine. This leads to slightly quicker local development and better support for offline development.

https://docs.gradle.org/current/userguide/repository_types.html#sub:maven_local
https://docs.gradle.org/current/userguide/dependency_cache.html#dependency_cache

@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.2-all.zip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move all the way to 5.0?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that would be good

Copy link
Owner

@leandroBorgesFerreira leandroBorgesFerreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cavega. Thanks for your contribution! I just suggested a small change and it is good to go.

Thanks @wokkaflokka for the revision!

app/build.gradle Outdated

implementation "io.reactivex.rxjava2:rxjava:2.1.10"
testImplementation 'junit:junit:4.12'
androidTestImplementation('androidx.test.espresso:espresso-core:3.1.0')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably remove the brackets for consistency.

@leandroBorgesFerreira
Copy link
Owner

This solves issue #90

@regogo
Copy link

regogo commented Jan 4, 2019

hi guys what release version will this fix be available? thanks

@cavega
Copy link
Contributor Author

cavega commented Jan 4, 2019

@leandroBorgesFerreira Holiday break. Will update PR soon. @regogo mavenLocal helps if you want to test changes locally on your machine outside of this project (i.e. on your own app project)

@regogo
Copy link

regogo commented Jan 7, 2019

Hey guys, is there a CircularProgress Button written in java? currently im having issues with 2.0.1 where in the CircularProgressButton is a kt file, can't extend it and the revertAnimation() cannot accept a java listener. Thanks

@wokkaflokka
Copy link
Contributor

@cavega needs rebase at this point.

@wokkaflokka
Copy link
Contributor

@regogo

Regarding the issue you are experiencing:

  1. That issue, as far as I can tell, existed before this PR.
  2. That issue could be resolved by making the class open, assuming the author of this library wants to make such a change.

@cavega
Copy link
Contributor Author

cavega commented Jan 7, 2019

@leandroBorgesFerreira Upgrading to Gradle 5.x requires upgrading the Android Gradle Plugin to one of the alpha versions. I don't know what's your policy on using non-stable library versions. I was getting errors even when using Android Gradle Plugin 3.4.0-alpha10 with gradle-5.1-milestone-1-all.zip. If it is ok with you, I rather leave that out of the scope of this PR. I'm addressing the other feedback right now.

@leandroBorgesFerreira
Copy link
Owner

leandroBorgesFerreira commented Jan 8, 2019

@regogo You can still use the listener, it is a function now. Like this:

CircularProgressButton btn = findViewById(R.id.javaBtn);
btn.revertAnimation(() -> null);

If you are not using Java 8, go like this:

CircularProgressButton btn = findViewById(R.id.javaBtn);
        btn.revertAnimation(new Function0<Unit>() {
            @Override
            public Unit invoke() {
                return null;
            }
        });

@leandroBorgesFerreira
Copy link
Owner

About the open class. I don't see any problem, just let's do it in another PR so we don't add too much entropy in this one.

build.gradle Outdated
@@ -27,6 +28,8 @@ allprojects {
repositories {
jcenter()
maven { url 'https://maven.google.com' }
google()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maven { url 'https://maven.google.com' } is equals to google()
I prefer to keep just google(). Please remove maven { url 'https://maven.google.com' }

https://stackoverflow.com/questions/46467561/difference-between-google-and-maven-url-https-maven-google-com

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed an update. I prefer google() also. Bypassed removing the longer statement.

@@ -3,5 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.3-all.zip

distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.3-all.zip
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just the removal of that extra empty line. I can undo it.

*/
class ViewVisibilityIdlingResource(private val view: View, private val expectedVisibility: Int) : IdlingResource {

private var mIdle: Boolean = false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library is not using Hungarian notation anymore. Just idle would be better =]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy/paste :) I'll update.

@@ -46,7 +46,7 @@
android:layout_marginLeft="18dp"
android:layout_marginRight="18dp"
android:background="@drawable/button_shape_no_fill"
android:text="Send"
android:text="@string/send"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! =D

val mainActivity = activityTestRule.launchActivity(Intent(app, MainActivity::class.java))

onView(withId(R.id.buttonTest1))
.check(matches(allOf<View>(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for <View>, Kotlin already infers the type.

import androidx.test.espresso.matcher.ViewMatchers.withEffectiveVisibility
import androidx.test.platform.app.InstrumentationRegistry
import androidx.test.rule.ActivityTestRule
import androidx.test.runner.AndroidJUnit4

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version is deprecated.

Use androidTestImplementation 'androidx.test.ext:junit:1.1.0' instead of androidTestImplementation 'androidx.test:runner:1.1.0'.

Take a look here: https://stackoverflow.com/questions/52776716/androidjunit4-class-is-deprecated-how-to-use-androidx-test-ext-junit-runners-an

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. FWIW in my experience, the RunWith annotation is unnecessary for a test like this.

Espresso.registerIdlingResources(ViewVisibilityIdlingResource(circularProgressButton, VISIBLE))

onView(withId(R.id.buttonTest2))
.check(matches(not<View>(withText(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This not made me confused. Shouldn't the test be visible after the spinner animation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree -- can we clarify the expected behavior? The title suggest text should be visible before and after spinner.

.check(matches(allOf<View>(
isDisplayed(), withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE), isEnabled())))

onView(withId(R.id.buttonTest2))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add this check to the above onView declaration if these assertions are both supposed to be on button2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for the performClick


onView(withId(R.id.buttonTest2))
.check(matches(withText(
containsString(mainActivity.resources.getString(R.string.send)))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the contains or can you use the resource directly? matches(withText(R.string.send))

IdlingResource that waits until the specified view matches the expected visibility criteria
Converted to Kotlin from original implementation found here: https://stackoverflow.com/a/35080198
*/
class ViewVisibilityIdlingResource(private val view: View, private val expectedVisibility: Int) : IdlingResource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw someone copy this over into another project recently. In my experience, using IdlingResource for views can be hokey and is explicitly against the Espresso guidelines (see: https://developer.android.com/training/testing/espresso/idling-resource#create-own)

My preference is towards a solution like this:

import android.view.View
import androidx.test.espresso.Espresso
import androidx.test.espresso.FailureHandler
import androidx.test.espresso.Root
import androidx.test.espresso.ViewAssertion
import androidx.test.espresso.matcher.RootMatchers
import org.hamcrest.Matcher
import org.joda.time.DateTimeUtils

class Wait private constructor(
        private val delay: Long,
        private val duration: Long,
        private val startTime: Long,
        private val viewMatcher: Matcher<View>,
        private val rootMatcher: Matcher<Root>,
        private val assertion: ViewAssertion): FailureHandler {

    init {
        checkAssertion()
    }

    override fun handle(error: Throwable?, viewMatcher: Matcher<View>?) {
        val currentTime = DateTimeUtils.currentTimeMillis()

        if (currentTime > (startTime + duration)) {
            throw WaitingTimeoutException(error)
        }

        try {
            Thread.sleep(delay)
        } catch (e: InterruptedException) {
            // let it go
        }

        checkAssertion()
    }

    private fun checkAssertion() {
        Espresso.onView(viewMatcher)
                .inRoot(rootMatcher)
                .withFailureHandler(this)
                .check(assertion);
    }

    class Builder constructor(private val viewMatcher: Matcher<View>) {
        private var delay: Long = 50
        private var duration: Long = 5000
        private var rootMatcher: Matcher<Root> = RootMatchers.DEFAULT

        fun inRoot(matcher: Matcher<Root>): Builder {
            this.rootMatcher = matcher;
            return this
        }

        fun delay(delay: Long): Builder {
            this.delay = delay
            return this
        }

        fun duration(duration: Long): Builder {
            this.duration = duration
            return this
        }

        fun until(assertion: ViewAssertion): Wait {
            val startTime = DateTimeUtils.currentTimeMillis()
            return Wait(delay, duration, startTime, viewMatcher, rootMatcher, assertion)
        }
    }

    companion object {
        @JvmStatic
        fun onView(viewMatcher: Matcher<View>): Builder {
            return Builder(viewMatcher)
        }
    }
}

class WaitingTimeoutException(cause: Throwable?) : RuntimeException(cause)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a loop with assertions in time to time, right? Uhm, for what I understand this will be slower than some version of TextInTextViewIdlyingResource... For me is looks a more natural solution. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you could set the delay to 1ms which would make a pretty tight loop.
That said, I personally would be less worried about optimal test speed and
instead settle for quick, functional, stable, and reproducible test runs.
I'm not opposed to either approach -- my inclination would be for the one
with minimal dev cost satisfying the above requirements. I have always
tried to avoid doing any view related waiting in an IdlingResource due to
the notes in the documentation and the fact that an IdlingResource should not
block the main thread.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IdlingResources are not blocking the UI thread.

There are also notes about implementing retry wrappers (in the section Identify when idling resources are needed).

I believe idling resources would a good solution since it is what a developer would expect when opening a PR in this part of the code. I saying this based on the fact that it is what I most commonly see.

About the note:

For example, the idling resources that you implement and register shouldn't contain references to View objects.

That makes sense when developing an Android app, since you should not wait for animations, you should test only business logic. But in the case of this library, the only place with any state is the View itself, so I don't think having a View inside the IdlingResource would be a problem to our use case.

Nevertheless both solution would work, I can accept any of then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, the IdlingResources are not blocking the UI thread here -- what I meant by that comment is that if you block the main thread in an idling resource, you can create deadlock in the test suite. W.r.t. the retry wrapper bit -- I see your point, my opinions has historically been something like "because this Wait uses the Espresso-provided error handling API, it's kosher". This opinion may be worth revisiting.

I think you've got a valuable opinion here and delegate to you/Carlos on this matter.

Another thing worth noting -- now that we are talking about this Espresso test, it's worth asking "when do these tests run?". Looking at the travis config, I don't think the androidTest artifacts are currently being compiled are run. Should that be added in the scope of this PR? I would lean towards the answer "yes", and suggest that the travis build should be updated to produce the test apk and run tests on Firebase Test Lab.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, right now the tests would not run on the CI since there's no configuration for it.

It would be great if you guys add it to the project. Firebase Test Lab is not an option, because it's not for free and I can't pay for it since I don't make any profit with this project. But it is still possible to run those tests in a emulator running in Travis.

it does make sense for add it in the scope of this PR, but I personally recommend doing it in another one so we don't put more requests in @cavega than he signed for.

I just opened an issue about this: #101

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way... there's an example of how to do it here: https://github.com/ebanx/swipe-button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

  1. I will update this build in a separate PR to run the tests.
  2. Since travis supports emulators, I'll evaluate that first (nice information for myself, I would note).
  3. If we had to end up at a paid solution such as FTL (based on the info you've given me, we won't) we could find a way to cover this for you -- for example, we could talk with our company about sponsoring this cost.

I'll move forward with adding tests to the build once this PR is finalized and merged.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seams great =].
If you don't want to wait for this PR to be merged, there's no problem to add a mock integration test just to confirm that it works. But that's up to you.
Thanks for the help!

onView(withId(R.id.buttonTest2)).perform(click())

val circularProgressButton = mainActivity.findViewById<CircularProgressButton>(R.id.buttonTest2)
Espresso.registerIdlingResources(ViewVisibilityIdlingResource(circularProgressButton, VISIBLE))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This idling resource is never waiting for anything, since the button is never going out of sight. Did you mean to wait for the text inside the button?
if that's the case, I am not sure the ViewVisibilityIdlingResource is going to work... the text inside the button is not a View, after all. We need to thing about a solution here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works:

class TextIdlingResource(private val view: TextView, private val expectedText: String) : IdlingResource {

    private var idle: Boolean = false
    private var resourceCallback: ResourceCallback? = null

    init {
        this.idle = false
        this.resourceCallback = null
    }

    override fun getName(): String {
        return TextIdlingResource::class.java.simpleName
    }

    override fun isIdleNow(): Boolean {
        idle = idle || view.text.toString() == expectedText

        if (idle) {
            if (resourceCallback != null) {
                resourceCallback!!.onTransitionToIdle()
            }
        }

        return idle
    }

    override fun registerIdleTransitionCallback(resourceCallback: ResourceCallback) {
        this.resourceCallback = resourceCallback
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using this, but I'm not sure the implementation works as expected. Unless the value of View#getText is changing here, this IdlingResource will immediately transition to idle for a matching view. The value of getText does not change along with view visibility.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it worked as I expected '.'
The text change during the animation, it is set to null, otherwise it wouldn't disappear. So the view.text should return null. But, now that I am thinking about this Idling resource... if we add a possibility to keep the text as the animation goes on, this idling resource won't make sense anymore... We should use the button state instead something that might change.

This version of the Idling Resource is better:

class ProgressButtonStateIdlingResource(
    private val progressButton: ProgressButton,
    private val expectedState: State = State.IDLE
) : IdlingResource {

    private var idle: Boolean = false
    private var resourceCallback: ResourceCallback? = null

    override fun getName(): String = progressButton::class.java.simpleName
   
    override fun isIdleNow(): Boolean {
        idle = idle || progressButton.getState() == expectedState

        if (idle) {
            if (resourceCallback != null) {
                resourceCallback!!.onTransitionToIdle()
            }
        }

        return idle
    }

    override fun registerIdleTransitionCallback(resourceCallback: ResourceCallback) {
        this.resourceCallback = resourceCallback
    }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the getState() is not available. I'll expose this method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 -- something keyed off the state sounds like the most robust solution for this specific use case. That said, the build is now passing, including the tests as they are currently written. I see a few paths forward from here:

  1. Review the PR again in it's current form, and proceed with no structural changes if approved.
  2. Update this PR to remove Wait and replace it with ProgressButtonStateIdlingResource.
  3. Move forward with the PR in it's current form and file an issue to capture the refactor to ProgressButtonStateIdlingResource.

Thoughts?

CHANGELOG.md Outdated
@@ -0,0 +1,14 @@
# Changelog

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use CHANGELOG in this library. We can track changes in the release part of Github

README.md Outdated
@@ -108,6 +108,10 @@ To avoid memory leaks is your code, you must dispose the buttons in the onDestro
progressButton.dispose()
}

## Changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use changelog in this library.

@Test
fun testTextVisibleBeforeAndAfterSpinnerAnimation() {
onView(withId(R.id.buttonTest2))
.check(matches(allOf<View>(isDisplayed(), isEnabled(), isClickable(), withText(R.string.send))))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for <View>, Kotlin already infers the type. This repeats along the code

@@ -5,7 +5,7 @@ apply plugin: 'com.github.dcendents.android-maven'
apply plugin: 'org.jmailen.kotlinter'

//Bintray depends on this global variable to set the library version!
version = "2.0.2"
version = "3.0.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version shouldn't be 3.0.0. There's no breaking changes since 2.0.0. The insertion of androidx was the breaking change in 1.14.0

.travis.yml Outdated
- lynx -dump app/build/outputs/gnag/gnag.html

after_success:
- ./gradle/buildWithTravis.sh
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script shouldn't be here. But I think it can be removed for now... It is not working in the moment

private var duration: Long = 5000
private var rootMatcher: Matcher<Root> = RootMatchers.DEFAULT

fun inRoot(matcher: Matcher<Root>): Builder {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply could be used here to shrink the code. It can be used in the whole builder instead of returning this

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example:

fun duration(duration: Long): Builder = apply {
    this.duration = duration
}

companion object {
@JvmStatic
fun onView(viewMatcher: Matcher<View>): Builder {
return Builder(viewMatcher)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Builder(viewMatcher)
fun onView(viewMatcher: Matcher<View>): Builder = Builder(viewMatcher)

@leandroBorgesFerreira
Copy link
Owner

I suggested some improvements in the Wait.kt file, but I still recommend the use of ProgressButtonStateIdlingResource, since there's still some work to be done in this PR... But feel free to choose between then, this PR is already big enough.

Carlos Vega II and others added 8 commits January 13, 2019 02:16
* Increase compileSdkVersion and targetSdkVersion to API 28

* Migrate all android.support libraries to the latest AndroidX equivalents.

* Update to androidx.constraintlayout:constraintlayout:1.1.3

* Update RxJava2 to version 2.2.3

* Added androidx.test:rules:1.1.1 which is the new depenency that defines ActivityTestRule

* Updated Kotiln to version 1.3.11

* Updated Android Gradle Plugin to 3.2.1 and Gradle to version 4.10.2
* CircularProgressButtonTest implements a few basic tests in regards to visibility of text before and after an animation is shown.

* ViewVisibilityIdlingResource allows test to wait until a given view matches the specified visibility criteria.

* Extracted Send string into a resource to avoid hardcoding values in tests.

* Added test runner dependency
* Removed CHANGELOG.md as it is not used by this project

* Reverted to patch version number

* Updated CircularProgressButtonTest by removing <View> casting and replacing Wait with ProgressButtonStateIdlingResource
* ProgressButtonStateIdlingResource: default Android Studio Kotlin tab code style is 8 spaces but lint expects 4; inlining for now

* CircularProgressButtonTest: lint does not like wildcard imports; explicitly specified ViewMatchers used.
Copy link
Owner

@leandroBorgesFerreira leandroBorgesFerreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seams great for me. Can I merge it?

Thanks @cavega and @wokkaflokka for the great work!

idle = idle || progressButton.getState() == expectedState

if (idle) {
if (resourceCallback != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resourceCallback?.onTransitionToIdle()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is made in the line above, so the !! is ok.

- sys-img-armeabi-v7a-android-22

before_install:
- chmod +x gradle/buildWithTravis.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildWithTravis is no longer used, it seems. How is uploading to bintray expected to work?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not currently working... I don't know if Travis-CI made some change in their API, but for now I'm publishing from my machine

@wokkaflokka
Copy link
Contributor

@leandroBorgesFerreira thank you as well. This change looks good to me. If you're comfortable with it, I would recommend merging.

@leandroBorgesFerreira leandroBorgesFerreira merged commit b14268a into leandroBorgesFerreira:master Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants