Grail

Merge lp://staging/~chasedouglas/grail/reject-test into lp://staging/grail

reject-test
Merge into trunk

Proposed by Chase Douglas on 2012-04-03

Status:	Merged
Merged at revision:	225
Proposed branch:	lp://staging/~chasedouglas/grail/reject-test
Merge into:	lp://staging/grail
Diff against target:	302 lines (+276/-1) 4 files modified test/Makefile.am (+2/-1) test/recordings/ntrig_dell_xt2/1_begin.record (+11/-0) test/recordings/ntrig_dell_xt2/1_end.record (+3/-0) test/x11/hold-reject.cpp (+260/-0)
To merge this branch:	bzr merge lp://staging/~chasedouglas/grail/reject-test
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Thomas Voß (community)		2012-04-03	Approve on 2012-04-03
Review via email: mp+100620@code.staging.launchpad.net

Description of the change

Regression test for bug 972012, where touches are not rejected until they end.

Revision history for this message

Thomas Voß (thomas-voss) wrote on 2012-04-03:

Looks good to me, two comments though:

(1.) Could you move

grail_subscription_deactivate(grail_handle(), subscription_);
grail_subscription_delete(subscription_);

from within the test into the fixture, either into TearDown or into the d'tor?

(2.) The fixture is missing a virtual d'tor.

review: Needs Fixing

Revision history for this message

Chase Douglas (chasedouglas) wrote on 2012-04-03:

> Looks good to me, two comments though:
>
> (1.) Could you move
>
> grail_subscription_deactivate(grail_handle(), subscription_);
> grail_subscription_delete(subscription_);
>
> from within the test into the fixture, either into TearDown or into the d'tor?

Yeah, it makes sense to put it there.

> (2.) The fixture is missing a virtual d'tor.

This is only needed if the class will be inherited. It's only used in this test, so this isn't necessary, IIRC.

Revision history for this message

Chase Douglas (chasedouglas) wrote on 2012-04-03:

> > (2.) The fixture is missing a virtual d'tor.
>
> This is only needed if the class will be inherited. It's only used in this
> test, so this isn't necessary, IIRC.

Thomas reminded me that the fixture class is inherited by the test, so this is needed.

Every fixture should be updated for this. However, this is only an issue in theory, since valgrind shows no leakage.

Revision history for this message

Thomas Voß (thomas-voss) wrote on 2012-04-03:

lgtm.

review: Approve

Revision history for this message

Chase Douglas (chasedouglas) wrote on 2012-04-03:

> > > (2.) The fixture is missing a virtual d'tor.
> >
> > This is only needed if the class will be inherited. It's only used in this
> > test, so this isn't necessary, IIRC.
>
> Thomas reminded me that the fixture class is inherited by the test, so this is
> needed.
>
> Every fixture should be updated for this. However, this is only an issue in
> theory, since valgrind shows no leakage.

Upon further reading, only the base class needs to have a virtual destructor. The derived classes will inherit the virtualness of the destructor automatically.

Since testing::~Test() is declared virtual in gtest/gtest.h, we don't need to do the same.

lp://staging/~chasedouglas/grail/reject-test updated on 2012-04-03

226. By Chase Douglas on 2012-04-03

Delete subscription in test fixture TearDown() method

The subscription is "owned" by the test fixture, so it should be deleted
by it, not by the test itself.