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

Assert drawables in list items #310

Merged
merged 14 commits into from
May 30, 2021

Conversation

gaelmarhic
Copy link
Contributor

This PR consists in adding a function to BaristaListAssertions that allows to assert that an item from a list contains a specific drawable.

Example:
assertDrawableDisplayedAtPosition(R.id.recycler, 0, R.id.imageview, R.drawable.ic_barista)

@alorma
Copy link
Member

alorma commented Jun 25, 2019

Hi @gaelmarhic

Just curios, have you tried to something similar to: https://github.com/SchibstedSpain/Barista#custom-assertions but applying it to list items? Maybe we can expand the assertXxxxAtPosition() methods, so we are able to do more magic on the future

@gaelmarhic
Copy link
Contributor Author

@alorma Thanks for reviewing my PR!
This looks good!
I don't have much time to look into it for now, but I definitely will when I can!
Should I close this PR?
Thanks!

@rocboronat
Copy link
Member

Hello! For me the solution is fine. @alorma's suggestion would lead to less code, which is always better in terms of maintainability, but as this is a gift, for me it is better than good 😄

The only issue I find is that there isn't a test to check that the assertion fails if the drawable is not found. Could you add it?

Thank you so much @gaelmarhic 👍

alorma
alorma previously approved these changes Sep 3, 2019
@gaelmarhic
Copy link
Contributor Author

@rocboronat Do you have an example of these kind of tests you are referring to?

@rocboronat
Copy link
Member

rocboronat commented Sep 16, 2019

@gaelmarhic I'm out of office for some time... maybe @SmasSive, @alorma or @Sloy could answer that question?

In the meantime, if you have the time, look for "fail" in the name of the tests. They are tests that check that your assertion would break if the check doesn't pass. We use to write the optimistic checks, that checks that it goes green when the assertion passes, but not the pessimistic ones, hihi

By the way, thanks again for the PR! 😊

@gaelmarhic
Copy link
Contributor Author

@rocboronat @alorma
I have added a test that checks that the assertion would break if the check doesn't pass as well as resolved the merge conflict in the README file.

@rocboronat
Copy link
Member

Hello @gaelmarhic!

It has been so difficult to join the whole Barista team in a meeting to talk about this. Sorry for the super late review!

We will go merging the PR, but eventually, we'll change your new method's name to assertDisplayedAtPosition(R.id.list, 0, R.id.imageview, R.drawable.ic_barista), just to keep it aligned with the existing API Barista offers.

Thanks a lot again! 🙇

@gaelmarhic
Copy link
Contributor Author

Thank you @rocboronat !

@Sloy Sloy enabled auto-merge (squash) May 30, 2021 17:13
@Sloy Sloy merged commit 5922d2a into AdevintaSpain:master May 30, 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.

4 participants