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

Reproduce issue #49 and fix it #50

Merged
merged 10 commits into from
May 10, 2017

Conversation

rocboronat
Copy link
Member

@rocboronat rocboronat commented May 5, 2017

This PR just reproduces this issue #49 and fixes it.

Thanks again for reporting it, @despinola !

@rocboronat rocboronat added the wip label May 5, 2017
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.

Great! 👍

RecyclerViewActions.actionOnItemAtPosition(position, clickChildWithId(itemToClickId)));
}

public static void clickRecyclerViewItemChild(@IdRes int recyclerViewId, int position, String text) {
onView(withId(recyclerViewId)).perform(
onView(allOf(withId(recyclerViewId), isDisplayed())).perform(
Copy link
Member

Choose a reason for hiding this comment

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

Idea: We could have some static method that constructs this kind of matcher, to avoid duplication of code.
That way we only have the Barista magic in one place. (Maybe two places, because assertions need a different matcher I think).

But this PR can go to production as it is and make people happy already!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Sloy ! Just done it on #51 , that applies the same rule over all Barista Actions. By the way, right now, I don't think that this could be a good idea on Assertions, cos we're trying to be aggressive on the assertion part of the test. We can be happy on the when, but not on the end, if we want to make reliable tests.

But I can be wrong. Let's try that approach and fix it if it doesn't work! 🎉

Choose a reason for hiding this comment

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

Sorry, I've not understood at all your explanation :(

Copy link
Member Author

@rocboronat rocboronat May 9, 2017

Choose a reason for hiding this comment

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

@despinola Let me try to improve my English... hehehe

@Sloy talked about creating a method that returns the allOf(...) Matcher, and also talked about applying it to Assertions.

Right now, we're only applying it to Actions, to fix the issue you described at #48 .

And then, focusing on applying the new Matcher on the Assertions, I think that it's not needed by now and, in the future, it could be an issue. So I suggest avoiding applying the new suggested method to Assertions, cos it could produce worse tests.

Choose a reason for hiding this comment

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

no no it's not your english.. I just needed more context :p
now it's much better, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for asking, @despinola ! :·)

import static com.schibsted.spain.barista.BaristaRecyclerViewActions.clickRecyclerViewItemChild;
import static com.schibsted.spain.barista.BaristaRecyclerViewActions.scrollTo;

@RunWith(AndroidJUnit4.class) public class RecyclerViewInsideViewPagerTest {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a guideline for formatting of class annotations? I think this is the only place where the class name and the runner are in the same line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just installed AndroidSquare codestyle in my machine and it seems that it formats like this. But let me move to the previous version... Actually, it was hard to me to find the public class statement some minutes ago :·/

@rocboronat
Copy link
Member Author

@Sloy done! Want to check it again?

Copy link

@despinola despinola left a comment

Choose a reason for hiding this comment

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

👍 I like it!
In case of being necessary, have you modified the documentation of the library?

@rocboronat
Copy link
Member Author

Thanks for asking @despinola ! I added explaining this magic at #7 , but, as #51 does the same thing than this PR but on all Barista's Actions, I propose to add it later :·)

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.

Nope, my comment was just about formatting. I trust your re-reformatting skills!

@rocboronat
Copy link
Member Author

💪

@rocboronat rocboronat merged commit b8c3344 into master May 10, 2017
@rocboronat rocboronat deleted the clicking-recycleritem-inside-viewpager branch May 10, 2017 09:11
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