Compiz

Merge lp://qastaging/~compiz-team/compiz/compiz.python_glib_tests_supressions_leaks into lp://qastaging/compiz/0.9.8

  1. compiz.python_glib_tests_supressions_leaks
  2. Merge into 0.9.8
Proposed by Sam Spilsbury
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 3316
Merged at revision: 3306
Proposed branch: lp://qastaging/~compiz-team/compiz/compiz.python_glib_tests_supressions_leaks
Merge into: lp://qastaging/compiz/0.9.8
Diff against target: 1577 lines (+1089/-47)
15 files modified
CMakeLists.txt (+3/-1)
compizconfig/compizconfig-python/src/compizconfig.pyx (+7/-2)
compizconfig/compizconfig-python/tests/compiz_config_test.py (+8/-5)
compizconfig/gsettings/tests/test_gsettings_tests.cpp (+43/-13)
compizconfig/gsettings/tests/test_gsettings_tests.h (+30/-0)
compizconfig/libcompizconfig/src/compiz.cpp (+3/-0)
compizconfig/libcompizconfig/src/iniparser.c (+2/-0)
compizconfig/libcompizconfig/src/main.c (+17/-3)
compizconfig/libcompizconfig/tests/test-ccs-object.cpp (+8/-0)
plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp (+21/-21)
src/pluginclasshandler/tests/test-pluginclasshandler.cpp (+10/-0)
src/timer/src/timer.cpp (+4/-0)
src/timer/tests/test-timer.cpp (+24/-0)
src/timer/tests/test-timer.h (+2/-2)
tests/experimental-memcheck/compiz.supp (+907/-0)
To merge this branch: bzr merge lp://qastaging/~compiz-team/compiz/compiz.python_glib_tests_supressions_leaks
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Francis Ginther Pending
Review via email: mp+118476@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-08-02.

Commit message

Make valgrind shut up about some known "leaks" in python and glib.
Fix some leaks in the tests in the process.

Description of the change

Makes valgrind shut up about some known "leaks" in python and glib.

Fix some leaks in the tests in the process.

Brings us down from

-- Processing memory checking output: #########
Memory checking results:
Memory Leak - 30
Potential Memory Leak - 15909
Uninitialized Memory Conditional - 10
Uninitialized Memory Read - 54

to

100% tests passed, 0 tests failed out of 213

Total Test time (real) = 320.27 sec
-- Processing memory checking output:
Memory checking results: (eg, zero)

Please note that set_tests_properties () and set_property (TEST foo PROPERTY ENVIRONMENT "FOO=BAR") are completely broken in CMake (they don't do anything). In addition, since we discover the tests at build-time and not cmake time, they won't work anyways. As such, I've had to define the variables in the tests themselves.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Works OK, but I don't think it's a good idea to name a class "General" (CCSGSettingsTestGeneral). That's quite vague.
Maybe "CCSGSettingsEnvTest"?

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Also needs a commit message.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

> Works OK, but I don't think it's a good idea to name a class "General"
> (CCSGSettingsTestGeneral). That's quite vague.
> Maybe "CCSGSettingsEnvTest"?

Sure, doing that.

Revision history for this message
Francis Ginther (fginther) wrote : Posted in a previous version of this proposal

Review was claimed by accident, please ignore.

review: Abstain
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Still missing changes requested on 2012-07-30

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Oh, whoops, bzr push must have failed. Re-pushed.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Sorry francis, you're still on the review for some reason. Just ignore it. ..

Revision history for this message
Francis Ginther (fginther) wrote : Posted in a previous version of this proposal

Review was claimed by accident, please ignore (2nd attempt :-) ).

review: Abstain
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Sorry I didn't notice this earlier...

Environment variables shouldn't be set in the C++ code at all. The test's environment should be set via the CMake test properties: http://www.cmake.org/cmake/help/v2.8.8/cmake.html#prop_test:ENVIRONMENT

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

In fact, I wanted to do that, however it is broken in CTest - setting those properties does absolutely nothing. As such, I've added it to the code directly. This is sub-optimal, but it works.

Resubmitted to reflect this.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

In addition to the above, set_tests_properties won't work anyways since we are introspecting the test binaries (CMake doesn't know about our tests).

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

OK then. Seems fine.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches