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

Update Espresso version #432

Merged
merged 8 commits into from
Aug 27, 2021
Merged

Update Espresso version #432

merged 8 commits into from
Aug 27, 2021

Conversation

JJaviMS
Copy link
Contributor

@JJaviMS JJaviMS commented Aug 9, 2021

Hi,

According to #430 I have updated the Espressor dependencies version. Due to a Dex error, caused by a too big Dex file, I had to add Multidex to the sample app.

I have also started to update some other versions, like the AndroidJUnit4 test runner and the Kotlin version (#431).

-Update Espresso versions
-Add to the sample app Multidex configuration since there was a Dex error after updating the Espresso versions
Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

Hello @JJaviMS! Sorry for the delay, I was on holidays until now 🌴
Thanks a lot for the PR. A dependency update was long overdue :)

I just added a couple of questions to make sure.

@@ -12,6 +12,7 @@ android {
defaultConfig {
minSdk = 19
targetSdk = 30
multiDexEnabled = true
Copy link
Member

Choose a reason for hiding this comment

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

Why was multidex enabled? Is the sampel app too big after the dependency changes?

If that's the case, it's probably easier to just bump the minSdk version to 21 and forget about multidex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know that using minSdk into 21 would fix that issue, if it does then I will bump the minSdk version and remove multidex!

Copy link
Member

Choose a reason for hiding this comment

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

Yes please, that would be great. Min SDK 21 is good enough for the sample.

@@ -24,10 +24,12 @@ android {
}

dependencies {
api("androidx.test.espresso:espresso-core:3.1.1")
api("androidx.test.espresso:espresso-contrib:3.1.1")
api("androidx.test.ext:junit:1.0.0")
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, why was this dependency added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that depency because I also updated the AndroidJUnit runner in androidx.test.runner.AndroidJUnit4 is deprecated and the new one is into androidx.test.ext.junit.runners.AndroidJUnit4. which depends on androidx.test.ext:junit.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid this dependency altogether if we remove all those @RunWith(AndroidJUnit4::class) annotations from the tests. They are not really needed.
Then, this dependency won't be needed, right? Do you want to do it? I can send a commit otherwise.

@JJaviMS
Copy link
Contributor Author

JJaviMS commented Aug 20, 2021

Hi!

I have removed the multidex and update the min SDK to 21. I just left the Barista minSdk in 19, but maybe we could bump it into 21 too.

Also I have removed the AndroidJunit4 runner from all the test.

Finally I have fixed some assertions test, those test failed because after updating the Espresso version some assertions messages changed, so I have changed all those messages.

Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @JJaviMS!

Just one last thing and we'll merge. Look at the VisibilityAssertionsTest file. I think it was accidentally re-formatted with the wrong indentation. Check out the Formatting section in the readme, we use a custom formatter for Barista.

If you don't mind fixing the indentation on that file, I think we'll be ready to merge :)

"View (with tag value: is \"presentTagValue\") didn't match condition (not is displayed on the screen to the user)";
assertThat(thrown).isInstanceOf(BaristaException.class).hasMessage(expectedMessage);
}
@Rule
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation

Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you so much @JJaviMS :)

@Sloy Sloy merged commit e688249 into AdevintaSpain:master Aug 27, 2021
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.

3 participants