-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add test device for CI in release process #417
Conversation
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
========================================
Coverage 68.2% 68.2%
Complexity 381 381
========================================
Files 68 68
Lines 2076 2076
Branches 163 163
========================================
Hits 1416 1416
Misses 570 570
Partials 90 90 |
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
============================================
+ Coverage 68.46% 68.64% +0.18%
- Complexity 395 396 +1
============================================
Files 69 69
Lines 2156 2156
Branches 171 171
============================================
+ Hits 1476 1480 +4
+ Misses 583 579 -4
Partials 97 97 |
@alfwatt are you able to review this PR? |
@Chaoba @chriswu42 sorry for the delay, we have some Android devs who can take a look at this one now @harvsu |
cloud_test.sh
Outdated
--device model=walleye,version=27,locale=en,orientation=portrait \ | ||
--device model=walleye,version=28,locale=en,orientation=portrait \ | ||
--device model=zeroflte,version=23,locale=en,orientation=portrait \ | ||
--device model=m0,version=18,locale=en,orientation=portrait \ |
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.
we should add this to the default list to address #412
@@ -18,6 +20,7 @@ | |||
public class AlarmMangerInstrumentationTest { | |||
|
|||
@Test | |||
@Ignore |
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.
let's not ignore, scheduleExact(...)
checks for >= API 19 or returns false
since it uses setExact(...)
, we can use set(...)
for < API 19. Let's also restrict this API for tests, something like
...
@RestrictTo(RestrictTo.Scope.TESTS)
boolean scheduleExact(long interval) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
manager.setExact(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime() + interval,
pendingIntent);
} else {
manager.set(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime() + interval,
pendingIntent);
}
return true;
}
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.
Does this guarantee the test to pass? This is because the test asserts the alarm at definite time and documentation for set() method does not talk about accuracy?
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.
set
became inexact
in API 19 and setExact
was added so prior to 19 set
should work.
https://developer.android.com/reference/android/app/AlarmManager#set(int,%20long,%20android.app.PendingIntent)
Docs - Applications whose targetSdkVersion is before API 19 will continue to get the previous alarm behavior: all of their scheduled alarms will be treated as exact.
@@ -63,8 +65,9 @@ public void checksScheduleExact() throws Exception { | |||
AlarmReceiver mockedAlarmReceiver = mock(AlarmReceiver.class); | |||
AlarmSchedulerFlusher theAlarmSchedulerFlusher = new AlarmSchedulerFlusher(mockedContext, mockedAlarmManager, | |||
mockedAlarmReceiver); | |||
|
|||
Assert.assertTrue(theAlarmSchedulerFlusher.scheduleExact(25)); | |||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) { |
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.
not required if we fix scheduleExact(...)
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.
🚀
Resolves #410
Instrumentation tests will run on more devices for release.