location-service

Merge lp:~mardy/location-service/lp1499256 into lp:location-service/trunk

  1. lp1499256
  2. Merge into trunk
Proposed by Alberto Mardegan on 2015-10-14
Status: Merged
Approved by: Thomas Voß on 2015-10-15
Approved revision: 218
Merge reported by: Thomas Voß
Merged at revision: not available
Proposed branch: lp:~mardy/location-service/lp1499256
Merge into: lp:location-service/trunk
Diff against target: 129 lines (+86/-3)
2 files modified
src/location_service/com/ubuntu/location/providers/gps/android_hardware_abstraction_layer.cpp (+3/-3)
tests/gps_provider_test.cpp (+83/-0)
To merge this branch: bzr merge lp:~mardy/location-service/lp1499256
Reviewer Review Type Date Requested Status
Thomas Voß (community) 2015-10-14 Approve on 2015-10-15
PS Jenkins bot continuous-integration Approve on 2015-10-15
Review via email: mp+274381@code.launchpad.net

Commit Message

Make sure that injected time is given in milliseconds

Description of the Change

Make sure that injected time is given in milliseconds

To post a comment you must log in.
Thomas Voß (thomas-voss) wrote :

Thanks for the changes. Two minor niggles inline.

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
lp:~mardy/location-service/lp1499256 updated on 2015-10-15
217. By Alberto Mardegan on 2015-10-15

Add comment

Alberto Mardegan (mardy) wrote :

On 10/14/2015 02:45 PM, Thomas Voß wrote:
> A singleton is fine here, I would however prefer: static MockHardwareGps *instance() { static MockHardwareGps inst; return &inst; }
> With that, you can get rid of custom ctor and dtor, as well as the static instance_.

I tried that, but it doesn't work because the object is instantiated in
the test as

   NiceMock<MockHardwareGps> hardwareGps;

and not via the MockHardwareGps::instance() method; therefore, two
objects are created.
A possibility would be to make the instance() method return a NiceMock'd
instance. Would you prefer that?

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Thomas Voß (thomas-voss) wrote :

> On 10/14/2015 02:45 PM, Thomas Voß wrote:
> > A singleton is fine here, I would however prefer: static MockHardwareGps
> *instance() { static MockHardwareGps inst; return &inst; }
> > With that, you can get rid of custom ctor and dtor, as well as the static
> instance_.
>
> I tried that, but it doesn't work because the object is instantiated in
> the test as
>
> NiceMock<MockHardwareGps> hardwareGps;
>
> and not via the MockHardwareGps::instance() method; therefore, two
> objects are created.
> A possibility would be to make the instance() method return a NiceMock'd
> instance. Would you prefer that?

Nah, stick with the instance member. Returning a NiceMock would require individual tests to manually verify and clear expectations as one global object would be reused for all tests. I think the benefit of your code is a fresh instance per test.

Some documentation would be great, though :)

lp:~mardy/location-service/lp1499256 updated on 2015-10-15
218. By Alberto Mardegan on 2015-10-15

Explain mock rationale

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Thomas Voß (thomas-voss) wrote :

Perfect, LGTM.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/location_service/com/ubuntu/location/providers/gps/android_hardware_abstraction_layer.cpp'
2--- src/location_service/com/ubuntu/location/providers/gps/android_hardware_abstraction_layer.cpp 2014-10-27 21:58:16 +0000
3+++ src/location_service/com/ubuntu/location/providers/gps/android_hardware_abstraction_layer.cpp 2015-10-15 09:16:52 +0000
4@@ -333,14 +333,14 @@
5 } else
6 {
7 auto now = location::Clock::now().time_since_epoch();
8- auto thiz = static_cast<android::HardwareAbstractionLayer*>(context);
9+ auto ms_since_epoch = std::chrono::duration_cast<std::chrono::milliseconds>(now);
10
11 static const int zero_uncertainty = 0;
12
13 u_hardware_gps_inject_time(
14 thiz->impl.gps_handle,
15- now.count(),
16- now.count(),
17+ ms_since_epoch.count(),
18+ ms_since_epoch.count(),
19 zero_uncertainty);
20 }
21 }
22
23=== modified file 'tests/gps_provider_test.cpp'
24--- tests/gps_provider_test.cpp 2015-01-12 08:41:14 +0000
25+++ tests/gps_provider_test.cpp 2015-10-15 09:16:52 +0000
26@@ -48,6 +48,31 @@
27
28 namespace
29 {
30+
31+struct MockHardwareGps
32+{
33+ MockHardwareGps()
34+ {
35+ instance_ = this;
36+ }
37+ ~MockHardwareGps()
38+ {
39+ instance_ = nullptr;
40+ }
41+
42+ MOCK_METHOD5(set_position_mode, bool(uint32_t, uint32_t, uint32_t, uint32_t, uint32_t));
43+ MOCK_METHOD3(inject_time, void(int64_t, int64_t, int));
44+
45+ static MockHardwareGps *mocked(UHardwareGps self) {
46+ return reinterpret_cast<MockHardwareGps*>(self);
47+ }
48+ static MockHardwareGps *instance() { return instance_; }
49+
50+ static MockHardwareGps *instance_;
51+};
52+
53+MockHardwareGps *MockHardwareGps::instance_ = nullptr;
54+
55 struct UpdateTrap
56 {
57 MOCK_METHOD1(on_position_updated, void(const location::Position&));
58@@ -211,6 +236,42 @@
59
60 }
61
62+/* Mock the hardware GPS platform API: the methods defined here will be invoked
63+ * instead of those exported by the system library.
64+ * We redefine these methods using the MockHardwareGps class above, which is
65+ * implemented using google-mock. This effectively allows us to test that the
66+ * right calls to the platform API are made.
67+ */
68+UHardwareGps
69+u_hardware_gps_new(UHardwareGpsParams *)
70+{
71+ using namespace ::testing;
72+
73+ return reinterpret_cast<UHardwareGps>(MockHardwareGps::instance());
74+}
75+
76+void
77+u_hardware_gps_delete(UHardwareGps)
78+{
79+}
80+
81+bool
82+u_hardware_gps_set_position_mode(UHardwareGps self, uint32_t mode, uint32_t recurrence,
83+ uint32_t min_interval, uint32_t preferred_accuracy,
84+ uint32_t preferred_time)
85+{
86+ MockHardwareGps *thiz = MockHardwareGps::mocked(self);
87+ return thiz->set_position_mode(mode, recurrence, min_interval,
88+ preferred_accuracy, preferred_time);
89+}
90+
91+void
92+u_hardware_gps_inject_time(UHardwareGps self, int64_t time, int64_t time_reference, int uncertainty)
93+{
94+ MockHardwareGps* thiz = MockHardwareGps::mocked(self);
95+ return thiz->inject_time(time, time_reference, uncertainty);
96+}
97+
98 TEST(AndroidGpsXtraDownloader, reading_configuration_from_valid_conf_file_works)
99 {
100 std::stringstream ss{gps_conf};
101@@ -261,6 +322,28 @@
102 provider.on_reference_location_updated(pos);
103 }
104
105+TEST(GpsProvider, time_requests_inject_current_time_into_the_hal)
106+{
107+ using namespace ::testing;
108+
109+ NiceMock<MockHardwareGps> hardwareGps;
110+
111+ gps::android::HardwareAbstractionLayer::Configuration configuration;
112+ gps::android::HardwareAbstractionLayer hal(configuration);
113+ std::shared_ptr<gps::HardwareAbstractionLayer> hal_ptr(&hal, [](gps::HardwareAbstractionLayer*){});
114+
115+ gps::Provider provider(hal_ptr);
116+ int64_t time = 0;
117+ EXPECT_CALL(hardwareGps, inject_time(_, _, 0)).Times(1).WillOnce(SaveArg<0>(&time));
118+
119+ auto t0 = std::chrono::duration_cast<std::chrono::milliseconds>(location::Clock::now().time_since_epoch());
120+
121+ gps::android::HardwareAbstractionLayer::on_request_utc_time(&hal);
122+
123+ auto t1 = std::chrono::duration_cast<std::chrono::milliseconds>(location::Clock::now().time_since_epoch());
124+ EXPECT_THAT(time, AllOf(Ge(t0.count()), Le(t1.count())));
125+}
126+
127 TEST(GpsProvider, updates_from_hal_are_passed_on_by_the_provider)
128 {
129 using namespace ::testing;

Subscribers

People subscribed via source and target branches