-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[New architecture] Improve viewmodel behaviour #2569
[New architecture] Improve viewmodel behaviour #2569
Conversation
|
||
// Check whether the dialog to create the public share has been properly closed | ||
onView(withText(R.string.share_via_link_create_title)).check(doesNotExist()) | ||
onView(withText(newPublicShare.name)).check(matches(isDisplayed())) | ||
|
||
unregisterCreateShareFragmentAsIdlingResource(createShareFragmentIdlingResource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you include this statement in a "@after" method??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the idling resource to unregister is created for each test and not shared between different ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@before" and "@after" are functions are called before and after running one single test.
My idea would be to have your tests as following:
private lateinit var idlingResource: <Your type>
@Before
fun setUp() {
idlingResource = // call constructor
//register ...
}
@After
fun tearDown() {
// unregister
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know what you mean, I tried that way but the tests were not working fine. Let me recheck it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@before
fun setUp() {
idlingResource = // call constructor
//register ...
}
I'm not using this because I register the idling resources in the middle of the tests, after executing other lines of code.
If you see the Expresso idling resources docs , there's two different ways to implement it (see below), I decided to use the second one and that's why I do not use @After
method either.
) | ||
IdlingPolicies.setIdlingResourceTimeout(1, TimeUnit.HOURS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 hour???? really???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, I forgot to change this
@@ -74,11 +78,11 @@ class OCShareViewModel( | |||
expirationDateInMillis: Long, | |||
permissions: Int, | |||
publicUpload: Boolean | |||
): LiveData<Resource<List<OCShare>>> = shareRepository.updatePublicShareForFile( | |||
): LiveData<Resource<Void>> = shareRepository.updatePublicShareForFile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use Void? Shouldn't it be better use Unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Since we are merging this branch into |
Implements #2565