Merge into development-branch : add-key-repeat-utility : Code : Mir

Status:	Work in progress
Proposed branch:	lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility
Merge into:	lp://qastaging/mir
Prerequisite:	lp://qastaging/~andreas-pokorny/mir/add-dispatchable-alarm-factory
Diff against target:	407 lines (+340/-11) 6 files modified include/test/mir/test/doubles/mock_alarm_factory.h (+18/-11) src/platforms/evdev/CMakeLists.txt (+1/-0) src/platforms/evdev/key_repeater.cpp (+97/-0) src/platforms/evdev/key_repeater.h (+64/-0) tests/unit-tests/input/CMakeLists.txt (+1/-0) tests/unit-tests/input/test_key_repeater.cpp (+159/-0)
To merge this branch:	bzr merge lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Needs Fixing on 2016-03-31
Kevin DuBois (community)		2016-03-23	Approve on 2016-03-30
Review via email: mp+289961@code.qastaging.launchpad.net

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-23:

#

+ // destructor cancels the alarm
+ ~MockAlarm()
+ {
+ cancel();
+ }
Seems specific to a test

+ if (existing_timer == repeat_alarms_by_scancode.end())
+ existing_timer = repeat_alarms_by_scancode.emplace(
+ std::make_pair(
+ code,
+ alarm_factory->create_alarm(
+ [this, code]()
+ {
+ this->handle_timeout(code);
+ })
+ )
+ ).first;

braces around multi-line if statement

+ mtd::MockAlarm * alarm = new mtd::MockAlarm;
needs delete/unique_ptr?

review: Needs Fixing

Reply

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-23:

#

> + mtd::MockAlarm * alarm = new mtd::MockAlarm;
> needs delete/unique_ptr?
Ah, reading through the mock factory, it seems this gets wrapped with a unique_ptr on invocation.... Still not threadsafe, and will leak if the test fails to call the function.

mir/test/gmock_fixes.h and .WillOnce(Invoke([]{ return std::make_unique<T>(); })) is the best way I've found to work around gmock's difficulty with move-only types.

Reply

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-23:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/642/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-23:

#

> > + mtd::MockAlarm * alarm = new mtd::MockAlarm;
> > needs delete/unique_ptr?
> Ah, reading through the mock factory, it seems this gets wrapped with a
> unique_ptr on invocation.... Still not threadsafe, and will leak if the test
> fails to call the function.

In some of the tests I want to define call expectations.. but when the unique_ptr is created inside the method I dont get access to the object. But I guess in those cases I could still create the Alarm object in advance with make_unique.

> mir/test/gmock_fixes.h and .WillOnce(Invoke([]{ return std::make_unique<T>();
> })) is the best way I've found to work around gmock's difficulty with move-
> only types.

Reply

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-23:

#

> + // destructor cancels the alarm
> + ~MockAlarm()
> + {
> + cancel();
> + }
> Seems specific to a test

But thats the behavior of Alarm objects..

> + if (existing_timer == repeat_alarms_by_scancode.end())
> + existing_timer = repeat_alarms_by_scancode.emplace(
> + std::make_pair(
> + code,
> + alarm_factory->create_alarm(
> + [this, code]()
> + {
> + this->handle_timeout(code);
> + })
> + )
> + ).first;
>
> braces around multi-line if statement
>
> + mtd::MockAlarm * alarm = new mtd::MockAlarm;
> needs delete/unique_ptr?

ack

Reply

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-24:

#

> > > + mtd::MockAlarm * alarm = new mtd::MockAlarm;
> > > needs delete/unique_ptr?
> > Ah, reading through the mock factory, it seems this gets wrapped with a
> > unique_ptr on invocation.... Still not threadsafe, and will leak if the test
> > fails to call the function.
>
> In some of the tests I want to define call expectations.. but when the
> unique_ptr is created inside the method I dont get access to the object. But I
> guess in those cases I could still create the Alarm object in advance with
> make_unique.
>
> > mir/test/gmock_fixes.h and .WillOnce(Invoke([]{ return
> std::make_unique<T>();
> > })) is the best way I've found to work around gmock's difficulty with move-
> > only types.

It does not work here. I had to reorder the expectations to still have a valid Alarm ptr in the test case. But to get the unique_ptr to the caller .. I cannot use Return() with a move only type, and an Invoke action with a mutable lambda.

Reply

lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility updated on 2016-03-24

3418. By Andreas Pokorny on 2016-03-24: fixed indenting

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-24:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/651/rebuild

FAILED: Continuous integration, rev:3418
https://mir-jenkins.ubuntu.com/job/mir-ci/651/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/592/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/628
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/620
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/620
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/602
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/602/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/602
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/602/artifact/output/*zip*/output.zip
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/602/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/602/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/602
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/602/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/602
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/602/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/651/rebuild

review: Needs Fixing (continuous-integration)

Reply

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-26:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/667/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-28:

#

> It does not work here. I had to reorder the expectations to still have a valid
> Alarm ptr in the test case. But to get the unique_ptr to the caller .. I
> cannot use Return() with a move only type, and an Invoke action with a mutable
> lambda.

can merge this branch in, makes things a bit more raii in the test: lp:~kdub/mir/key-repeat-gmock-fixes

Reply

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-30:

#

wow.. for some reason I always tried binding the alarm object by value to lambda.
Works fine. Thanks!

Reply

lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility updated on 2016-03-30

3419. By Andreas Pokorny on 2016-03-30: merge prereq
3420. By Andreas Pokorny on 2016-03-30: Merge improvement from kdub..

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-30:

#

lgtm

review: Approve

Reply

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-31:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/709/rebuild

review: Needs Fixing (continuous-integration)

Reply

lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility updated on 2016-03-31

3421. By Andreas Pokorny on 2016-03-31: merge prereq to resolve conflict

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-31:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/716/rebuild

review: Needs Fixing (continuous-integration)

Reply

Mir

Merge lp://qastaging/~andreas-pokorny/mir/add-key-repeat-utility into lp://qastaging/mir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers