Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306333002:
[sensors] Add Generic Sensor platform unit tests. (Closed)
|
Created:
4 years, 3 months ago by maksims (do not use this acc) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Add Generic Sensor platform unit tests.
This patch adds platform side unit tests for generic
sensor API.
BUG=606766
Committed: https://crrev.com/d14d9caac146d4fb9cec5888448a8728a9326c72
Cr-Commit-Position: refs/heads/master@{#421495}
Patch Set 1 : Generic Sensor Unittest #
Total comments: 24
Patch Set 2 : Comments #
Total comments: 25
Patch Set 3 : Mikhail's comments #
Total comments: 36
Patch Set 4 : Rebased, Tim's comments #Patch Set 5 : More comments #Patch Set 6 : mock->fake naming #
Total comments: 5
Patch Set 7 : New test and config map getter instead of friend class #
Total comments: 4
Patch Set 8 : Tim's comments #
Messages
Total messages: 134 (114 generated)
Description was changed from ========== Generic Sensor Unittest BUG= ========== to ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. The following files are taken from another cl (https://codereview.chromium.org/2284613002/) to upload the patch: device/generic_sensor/platform_sensor.h device/generic_sensor/platform_sensor.cc device/generic_sensor/platform_sensor_configuration.h device/generic_sensor/platform_sensor_configuration.cc ==========
maksim.sisov@intel.com changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com, timvolodine@chromium.org
Description was changed from ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. The following files are taken from another cl (https://codereview.chromium.org/2284613002/) to upload the patch: device/generic_sensor/platform_sensor.h device/generic_sensor/platform_sensor.cc device/generic_sensor/platform_sensor_configuration.h device/generic_sensor/platform_sensor_configuration.cc ========== to ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. The following files are taken from another cl (https://codereview.chromium.org/2284613002/) to upload the patch: > device/generic_sensor/platform_sensor.h > device/generic_sensor/platform_sensor.cc > device/generic_sensor/platform_sensor_configuration.h > device/generic_sensor/platform_sensor_configuration.cc ==========
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
please review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gentle reminder
https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:38: if (configuration.frequency() > kMaxFrequencyValue) return configuration.frequency() <= kMaxFrequencyValue; ? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:10: #include "base/memory/singleton.h" Is this needed? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:74: friend class MockPlatformSensor; Is this needed?
Pls. exclude the other CL's files, just put dependency to https://codereview.chromium.org/2284613002/ instead. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:38: if (configuration.frequency() > kMaxFrequencyValue) return configuration.frequency() <= kMaxFrequencyValue; https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:43: void MockPlatformSensor::NotifySensorReadingChanged() { why override this method? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:47: void MockPlatformSensor::NotifySensorError() { ditto. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:24: bool IsStarted() { return started_; } bool started() const {} https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:29: const ConfigMap& GetConfigMap() { return config_map_; } const ConfigMap& config_map() const {} https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:39: static constexpr double kMaxFrequencyValue = 50.0; why not use PlatformSensorConfiguration::kMaxAllowedFrequency ? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.cc:17: MockPlatformSensorProvider::MockPlatformSensorProvider() {} = default; https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:14: static const uint64_t kReadBufferSize = 32ULL; that can be taken from mojom::SensorReadBuffer https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:20: const mojom::SensorType kPressure = mojom::SensorType::PRESSURE; let's not hide the actual types, would 'using mojom::SensorType' suffice? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:180: std::unique_ptr<PlatformSensorTestClient> clients[num]; here and below, pls. use std::vector<std::unique_ptr<PlatformSensorTestClient>> instead.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:38: if (configuration.frequency() > kMaxFrequencyValue) On 2016/09/07 10:46:19, Mikhail wrote: > return configuration.frequency() <= kMaxFrequencyValue; Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:38: if (configuration.frequency() > kMaxFrequencyValue) On 2016/09/07 09:53:01, shalamov wrote: > return configuration.frequency() <= kMaxFrequencyValue; ? Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.cc:43: void MockPlatformSensor::NotifySensorReadingChanged() { On 2016/09/07 10:46:19, Mikhail wrote: > why override this method? I cannot access them outside this code. Those methods are protected. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:10: #include "base/memory/singleton.h" On 2016/09/07 09:53:01, shalamov wrote: > Is this needed? Put it in wrong header. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:24: bool IsStarted() { return started_; } On 2016/09/07 10:46:19, Mikhail wrote: > bool started() const {} Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:29: const ConfigMap& GetConfigMap() { return config_map_; } On 2016/09/07 10:46:19, Mikhail wrote: > const ConfigMap& config_map() const {} Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:39: static constexpr double kMaxFrequencyValue = 50.0; On 2016/09/07 10:46:19, Mikhail wrote: > why not use PlatformSensorConfiguration::kMaxAllowedFrequency ? I though a bit different way. PlatformSensorConfiguration::kMaxAllowedFrequency seems like a common frequency restriction for all the sensors, but then each sensor can have it's own restriction, which is less that PlatformSensorConfiguration::kMaxAllowedFrequency. Isn't it so? https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.cc:17: MockPlatformSensorProvider::MockPlatformSensorProvider() {} On 2016/09/07 10:46:19, Mikhail wrote: > = default; Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:74: friend class MockPlatformSensor; On 2016/09/07 09:53:01, shalamov wrote: > Is this needed? Otherwise I cannot get config_map_. It's private. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:14: static const uint64_t kReadBufferSize = 32ULL; On 2016/09/07 10:46:19, Mikhail wrote: > that can be taken from mojom::SensorReadBuffer Done. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:20: const mojom::SensorType kPressure = mojom::SensorType::PRESSURE; On 2016/09/07 10:46:19, Mikhail wrote: > let's not hide the actual types, would 'using mojom::SensorType' suffice? Done.
gentle reminder
https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:24: void NotifySensorReadingChanged(); wouldn't 'using PlatformSensor::NotifySensorReadingChanged()' help here? https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:25: void NotifySensorError(); ditto https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:37: static constexpr double kMaxFrequencyValue = 50.0; if you want too keep is as a limit for the test, it should be renamed so that it does not clash with the same member from PlatformSensorConfiguration. kMaxFrequencyValueForTests or smth like that https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:45: void set_suspended(bool value) { notification_suspended_ = value; } set_notification_suspended() https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:47: void set_sensor(scoped_refptr<PlatformSensor> sensor) { SetSensor // not a trivial setter https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:52: bool IsSensorReadingChanged() { return sensor_reading_changed_; } bool is_sensor_reading_changed() const // trivial getter https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:54: bool SensorHasError() { return sensor_error_; } ditto https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: void remove_sensor(scoped_refptr<PlatformSensor> sensor) { this method is not needed, client code can call sensor_->RemoveClient(sensor_client); https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:71: sensor_client_.reset(new PlatformSensorTestClient()); : sensor_client_(new PlatformSensorTestClient()) https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:73: EXPECT_TRUE(sensor_provider_); I don't think we should test standard singleton classes https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:78: return sensor_provider_->CreateSensor( MockPlatformSensorProvider::GetInstance()->CreateSensor(.. drop 'PlatformSensorProvider* sensor_provider_;' https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:87: scoped_refptr<PlatformSensor> sensor1 = sensor_provider_->CreateSensor( why not call 'CreateSensor(SensorType::AMBIENT_LIGHT)' ? (in other places as well) https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:183: std::unique_ptr<PlatformSensorTestClient> clients[num]; pls don't make arrays with smart pointers. https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:24: void NotifySensorReadingChanged(); On 2016/09/12 08:12:43, Mikhail wrote: > wouldn't 'using PlatformSensor::NotifySensorReadingChanged()' help here? I don't get it. NotifySensorReadingChanged() is protected member of PlatformSensor. Are there other ways how to access it? I'm calling it in the unittests with mock_sensor_->NotifySensorReadingChanged(); https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:37: static constexpr double kMaxFrequencyValue = 50.0; On 2016/09/12 08:12:43, Mikhail wrote: > if you want too keep is as a limit for the test, it should be renamed so that it > does not clash with the same member from PlatformSensorConfiguration. > kMaxFrequencyValueForTests or smth like that Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:45: void set_suspended(bool value) { notification_suspended_ = value; } On 2016/09/12 08:12:43, Mikhail wrote: > set_notification_suspended() Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:47: void set_sensor(scoped_refptr<PlatformSensor> sensor) { On 2016/09/12 08:12:44, Mikhail wrote: > SetSensor // not a trivial setter Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:52: bool IsSensorReadingChanged() { return sensor_reading_changed_; } On 2016/09/12 08:12:43, Mikhail wrote: > bool is_sensor_reading_changed() const // trivial getter Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:54: bool SensorHasError() { return sensor_error_; } On 2016/09/12 08:12:44, Mikhail wrote: > ditto https://google.github.io/styleguide/cppguide.html#Function_Names Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: void remove_sensor(scoped_refptr<PlatformSensor> sensor) { On 2016/09/12 08:12:43, Mikhail wrote: > this method is not needed, client code can call > sensor_->RemoveClient(sensor_client); Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:71: sensor_client_.reset(new PlatformSensorTestClient()); On 2016/09/12 08:12:43, Mikhail wrote: > : sensor_client_(new PlatformSensorTestClient()) Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:73: EXPECT_TRUE(sensor_provider_); On 2016/09/12 08:12:43, Mikhail wrote: > I don't think we should test standard singleton classes Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:78: return sensor_provider_->CreateSensor( On 2016/09/12 08:12:43, Mikhail wrote: > MockPlatformSensorProvider::GetInstance()->CreateSensor(.. > drop 'PlatformSensorProvider* sensor_provider_;' Done. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:87: scoped_refptr<PlatformSensor> sensor1 = sensor_provider_->CreateSensor( On 2016/09/12 08:12:44, Mikhail wrote: > why not call 'CreateSensor(SensorType::AMBIENT_LIGHT)' ? (in other places as > well) I've forgotten to modify the callers here. First, I wanted to created sensors with wrong buffer sizes and offsets, but it turned CHECK is placed to the real code and tests just crash. https://codereview.chromium.org/2306333002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:183: std::unique_ptr<PlatformSensorTestClient> clients[num]; On 2016/09/12 08:12:43, Mikhail wrote: > pls don't make arrays with smart pointers. > https://codereview.chromium.org/2306333002/diff/100001/device/generic_sensor/... Done.
lgtm pls update the CL description (it does not contain files from https://codereview.chromium.org/2284613002/ any more) https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:22: bool started() { return started_; } nit: const method https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:49: void set_sensor(scoped_refptr<PlatformSensor> sensor) { SetSensor
https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:12: class MockPlatformSensor : public PlatformSensor { FakePlatformSensor? https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:48: #endif /* DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_H_ */ #endif // DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_H_ and also elsewhere https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:12: class MockPlatformSensorProvider : public PlatformSensorProvider { I think we usually call these type of classes "fakes" instead of "mocks". from the gmock documentation: " Note: It is easy to confuse the term fake objects with mock objects. Fakes and mocks actually mean very different things in the Test-Driven Development (TDD) community: Fake objects have working implementations, but usually take some shortcut (perhaps to make the operations less expensive), which makes them not suitable for production. An in-memory file system would be an example of a fake. Mocks are objects pre-programmed with expectations, which form a specification of the calls they are expected to receive. " https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:12: class MockPlatformSensorProvider : public PlatformSensorProvider { also small "fake" test classes can be placed inside the actual unittest if that is more convenient (don't know if that's the case here) https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:31: #endif /* DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_PROVIDER_H_ */ // https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:54: bool is_sensor_reading_changed() const { return sensor_reading_changed_; } sensor_reading_changed() i.e. trivial getter https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: bool sensor_has_error() const { return sensor_error_; } just sensor_error() ? https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:59: scoped_refptr<PlatformSensor> sensor_; is there a reason this cannot be a unique_ptr? https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:80: scoped_refptr<PlatformSensor> sensor1 = same here, why not use unique_ptr? and also everywhere below https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:111: EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); can we also have a test for leaks? something like: scoped_refptr<PlatformSensor> sensor1 = CreateSensor(SensorType::AMBIENT_LIGHT); EXPECT_TRUE(sensor1); EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); sensor1 = nullptr; EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT)->GetType()); https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:233: const double frq = 30; any chance of a better name? probably not necessary to have a separate variable at all if it's used only once below https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:279: double frq = 30; same here regarding naming https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:280: PlatformSensorConfiguration config(frq++); why frq++ here?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. The following files are taken from another cl (https://codereview.chromium.org/2284613002/) to upload the patch: > device/generic_sensor/platform_sensor.h > device/generic_sensor/platform_sensor.cc > device/generic_sensor/platform_sensor_configuration.h > device/generic_sensor/platform_sensor_configuration.cc ========== to ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. ==========
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #5 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:280001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:340001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #4 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:12: class MockPlatformSensor : public PlatformSensor { On 2016/09/13 19:38:53, timvolodine wrote: > FakePlatformSensor? We better go with Mock names. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:22: bool started() { return started_; } On 2016/09/13 10:31:16, Mikhail wrote: > nit: const method Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor.h:48: #endif /* DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_H_ */ On 2016/09/13 19:38:52, timvolodine wrote: > #endif // DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_H_ > > and also elsewhere Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:12: class MockPlatformSensorProvider : public PlatformSensorProvider { On 2016/09/13 19:38:53, timvolodine wrote: > I think we usually call these type of classes "fakes" instead of "mocks". > > from the gmock documentation: > " > Note: It is easy to confuse the term fake objects with mock objects. Fakes and > mocks actually mean very different things in the Test-Driven Development (TDD) > community: > > Fake objects have working implementations, but usually take some shortcut > (perhaps to make the operations less expensive), which makes them not suitable > for production. An in-memory file system would be an example of a fake. > Mocks are objects pre-programmed with expectations, which form a specification > of the calls they are expected to receive. > " Good point, but these classes more or less resemble the real code. Thus, I think we better stick with this kind of naming. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:31: #endif /* DEVICE_GENERIC_SENSOR_MOCK_PLATFORM_SENSOR_PROVIDER_H_ */ On 2016/09/13 19:38:53, timvolodine wrote: > // Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:49: void set_sensor(scoped_refptr<PlatformSensor> sensor) { On 2016/09/13 10:31:17, Mikhail wrote: > SetSensor Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:54: bool is_sensor_reading_changed() const { return sensor_reading_changed_; } On 2016/09/13 19:38:53, timvolodine wrote: > sensor_reading_changed() > i.e. trivial getter Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: bool sensor_has_error() const { return sensor_error_; } On 2016/09/13 19:38:53, timvolodine wrote: > just sensor_error() ? Well, if I name this method sensor_error(), I would need to return actual error instead. But this one just says whether there has been an error or not. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:59: scoped_refptr<PlatformSensor> sensor_; On 2016/09/13 19:38:53, timvolodine wrote: > is there a reason this cannot be a unique_ptr? It's implementation specific. PlatformSensor returns scoped_refptr's because of multithreading issues. For more details, please ask Mikhail or Alexander. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:80: scoped_refptr<PlatformSensor> sensor1 = On 2016/09/13 19:38:53, timvolodine wrote: > same here, why not use unique_ptr? and also everywhere below answered above. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:111: EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); On 2016/09/13 19:38:53, timvolodine wrote: > can we also have a test for leaks? something like: > > scoped_refptr<PlatformSensor> sensor1 = > CreateSensor(SensorType::AMBIENT_LIGHT); > EXPECT_TRUE(sensor1); > EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); > > sensor1 = nullptr; > EXPECT_EQ(SensorType::AMBIENT_LIGHT, > sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT)->GetType()); No, sensor1 is scoped_refptr. If it is reassigned to nullptr, the test just crashes. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:279: double frq = 30; On 2016/09/13 19:38:53, timvolodine wrote: > same here regarding naming Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:280: PlatformSensorConfiguration config(frq++); On 2016/09/13 19:38:53, timvolodine wrote: > why frq++ here? Nothing special. Just wanted to have different clients with different frequencies, but it doesn't make any sense to test this here.
Description was changed from ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the generic sensor API. ========== to ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the platform side of generic sensor API. ==========
Description was changed from ========== [sensors] Add Generic Sensor platform unittests. This patch adds unittests for the platform side of generic sensor API. ========== to ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. ==========
https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:12: class MockPlatformSensorProvider : public PlatformSensorProvider { On 2016/09/16 09:22:19, maksims wrote: > On 2016/09/13 19:38:53, timvolodine wrote: > > I think we usually call these type of classes "fakes" instead of "mocks". > > > > from the gmock documentation: > > " > > Note: It is easy to confuse the term fake objects with mock objects. Fakes and > > mocks actually mean very different things in the Test-Driven Development (TDD) > > community: > > > > Fake objects have working implementations, but usually take some shortcut > > (perhaps to make the operations less expensive), which makes them not suitable > > for production. An in-memory file system would be an example of a fake. > > Mocks are objects pre-programmed with expectations, which form a specification > > of the calls they are expected to receive. > > " > > Good point, but these classes more or less resemble the real code. Thus, I think > we better stick with this kind of naming. Why? Is there any reason it should be called a 'mock'? Looks like your classes provide a very simple implementation which by definition is a 'fake' (as described above). https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: bool sensor_has_error() const { return sensor_error_; } On 2016/09/16 09:22:20, maksims wrote: > On 2016/09/13 19:38:53, timvolodine wrote: > > just sensor_error() ? > > Well, if I name this method sensor_error(), I would need to return actual error > instead. But this one just says whether there has been an error or not. you are returning sensor_error_ which is a boolean already (which makes it a trivial getter) https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:111: EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); On 2016/09/16 09:22:19, maksims wrote: > On 2016/09/13 19:38:53, timvolodine wrote: > > can we also have a test for leaks? something like: > > > > scoped_refptr<PlatformSensor> sensor1 = > > CreateSensor(SensorType::AMBIENT_LIGHT); > > EXPECT_TRUE(sensor1); > > EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); > > > > sensor1 = nullptr; > > EXPECT_EQ(SensorType::AMBIENT_LIGHT, > > sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT)->GetType()); > > No, sensor1 is scoped_refptr. If it is reassigned to nullptr, the test just > crashes. why? should it crash? it's refptr so I would expect I can nullify it and then get back again via GetSensor()? The reason I actually asked for this test is because I was worried there may be a leak in: https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor_pr... because a raw pointer is stored in map, so just making sure.. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:233: const double frq = 30; On 2016/09/13 19:38:53, timvolodine wrote: > any chance of a better name? probably not necessary to have a separate variable > at all if it's used only once below ?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/mock_platform_sensor_provider.h (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/mock_platform_sensor_provider.h:12: class MockPlatformSensorProvider : public PlatformSensorProvider { On 2016/09/19 17:14:29, timvolodine wrote: > On 2016/09/16 09:22:19, maksims wrote: > > On 2016/09/13 19:38:53, timvolodine wrote: > > > I think we usually call these type of classes "fakes" instead of "mocks". > > > > > > from the gmock documentation: > > > " > > > Note: It is easy to confuse the term fake objects with mock objects. Fakes > and > > > mocks actually mean very different things in the Test-Driven Development > (TDD) > > > community: > > > > > > Fake objects have working implementations, but usually take some shortcut > > > (perhaps to make the operations less expensive), which makes them not > suitable > > > for production. An in-memory file system would be an example of a fake. > > > Mocks are objects pre-programmed with expectations, which form a > specification > > > of the calls they are expected to receive. > > > " > > > > Good point, but these classes more or less resemble the real code. Thus, I > think > > we better stick with this kind of naming. > > Why? Is there any reason it should be called a 'mock'? Looks like your classes > provide a very simple implementation which by definition is a 'fake' (as > described above). OK, I went through mock and fakes and it seems like we better use "fake_*" names here. Done! https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:56: bool sensor_has_error() const { return sensor_error_; } On 2016/09/19 17:14:29, timvolodine wrote: > On 2016/09/16 09:22:20, maksims wrote: > > On 2016/09/13 19:38:53, timvolodine wrote: > > > just sensor_error() ? > > > > Well, if I name this method sensor_error(), I would need to return actual > error > > instead. But this one just says whether there has been an error or not. > > you are returning sensor_error_ which is a boolean already (which makes it a > trivial getter) Done. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:111: EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); On 2016/09/19 17:14:29, timvolodine wrote: > On 2016/09/16 09:22:19, maksims wrote: > > On 2016/09/13 19:38:53, timvolodine wrote: > > > can we also have a test for leaks? something like: > > > > > > scoped_refptr<PlatformSensor> sensor1 = > > > CreateSensor(SensorType::AMBIENT_LIGHT); > > > EXPECT_TRUE(sensor1); > > > EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType()); > > > > > > sensor1 = nullptr; > > > EXPECT_EQ(SensorType::AMBIENT_LIGHT, > > > sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT)->GetType()); > > > > No, sensor1 is scoped_refptr. If it is reassigned to nullptr, the test just > > crashes. > > why? should it crash? it's refptr so I would expect I can nullify it and then > get back again via GetSensor()? > > The reason I actually asked for this test is because I was worried there may be > a leak in: > https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor_pr... > because a raw pointer is stored in map, so just making sure.. Please check now. I've added this case here. https://codereview.chromium.org/2306333002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:233: const double frq = 30; On 2016/09/19 17:14:29, timvolodine wrote: > On 2016/09/13 19:38:53, timvolodine wrote: > > any chance of a better name? probably not necessary to have a separate > variable > > at all if it's used only once below > > ? removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks, two more comments below https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:76: friend class FakePlatformSensor; if it's only to return the config map, maybe just add a config_map getter to protected section (with the same comment "for testing purposes")? https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:135: sensor1 = nullptr; this doesn't test for leaks because you are still holding on to AMBIENT_LIGHT in sensor3. Could you please extract this into a separate test with only one sensor?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... device/generic_sensor/platform_sensor.h:76: friend class FakePlatformSensor; On 2016/09/21 13:07:48, timvolodine wrote: > if it's only to return the config map, maybe just add a config_map getter to > protected section (with the same comment "for testing purposes")? Done. https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:135: sensor1 = nullptr; On 2016/09/21 13:07:48, timvolodine wrote: > this doesn't test for leaks because you are still holding on to AMBIENT_LIGHT in > sensor3. Could you please extract this into a separate test with only one > sensor? Ah, OK! I got your idea. Basically, this leak should never happen. As long as when all clients release their references to a certain sensor, the destructor of that sensor calls provider_->RemoveSensor(GetType()); which removes that raw pointer you've mentioned before. Of course, I've added a separate test for this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. ========== to ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. BUG=606766 ==========
lgtm % comments https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/400001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:135: sensor1 = nullptr; On 2016/09/26 07:09:15, maksims wrote: > On 2016/09/21 13:07:48, timvolodine wrote: > > this doesn't test for leaks because you are still holding on to AMBIENT_LIGHT > in > > sensor3. Could you please extract this into a separate test with only one > > sensor? > > Ah, OK! I got your idea. Basically, this leak should never happen. As long as > when all clients release their references to a certain sensor, the destructor of > that sensor calls provider_->RemoveSensor(GetType()); which removes that raw > pointer you've mentioned before. > > Of course, I've added a separate test for this. > yeah the construction with the destructor is a bit weird (and the fact that GetSensor can return null at any stage). I think I would actually prefer direct ownership or something like that, but it seems that's outside of the scope of this patch anyway. https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... File device/generic_sensor/fake_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... device/generic_sensor/fake_platform_sensor.h:26: using PlatformSensor::config_map; is this needed? https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:150: // This should never run. If this should never run why is it here? EXPECT_FALSE(sensor2) should be sufficient.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... File device/generic_sensor/fake_platform_sensor.h (right): https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... device/generic_sensor/fake_platform_sensor.h:26: using PlatformSensor::config_map; On 2016/09/27 19:11:00, timvolodine wrote: > is this needed? Yes, this is a protected method. Otherwise, fake_sensor_->config_map(); cannot be called. https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_unittest.cc (right): https://codereview.chromium.org/2306333002/diff/420001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_unittest.cc:150: // This should never run. On 2016/09/27 19:11:00, timvolodine wrote: > If this should never run why is it here? EXPECT_FALSE(sensor2) should be > sufficient. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikhail.pozdnyakov@intel.com, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2306333002/#ps440001 (title: "Tim's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. BUG=606766 ========== to ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. BUG=606766 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. BUG=606766 ========== to ========== [sensors] Add Generic Sensor platform unit tests. This patch adds platform side unit tests for generic sensor API. BUG=606766 Committed: https://crrev.com/d14d9caac146d4fb9cec5888448a8728a9326c72 Cr-Commit-Position: refs/heads/master@{#421495} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d14d9caac146d4fb9cec5888448a8728a9326c72 Cr-Commit-Position: refs/heads/master@{#421495} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
