Created attachment 229703 [details] [review]
Add test suite

Uploaded it as a patch to start the review.

Review of attachment 229703 [details] [review]:

::: plugins/power/test.py
@@ +53,3 @@
+        '''Start minimal GNOME session'''
+
+        klass.session_dir = os.path.expanduser('~/.config/gnome-session/sessions')

We shouldn't touch the user's setup or files. Reassign the XDG _DATA_DIRS/etc. to avoid this.

@@ +63,3 @@
+''')
+
+        klass.app_dir = os.path.expanduser('~/.local/share/applications')

Ditto.

@@ +73,3 @@
+
+        klass.session_log = tempfile.NamedTemporaryFile()
+        klass.session = subprocess.Popen(['gnome-session', '--session=null', '--debug'],

At the end of the day, I would rather we tested against a mock gnome-session by default, so as to avoid API change problems (as in, we're compiling a version that's newer than the one installed).

@@ +118,3 @@
+
+    def start_consolekit(self):
+        '''start mock ConsoleKit'''

The ConsoleKit stuff would obviously have to get replaced or supplemented by the systemd support.

@@ +169,3 @@
+    '''Test the power plugin'''
+
+    def setUp(self):

Is this instantiated for each test?

For bug 688878 for example, I'd need to change the "CanHibernate" to something else.

@@ +215,3 @@
+        self.daemon_log = tempfile.NamedTemporaryFile()
+        self.daemon = subprocess.Popen(
+            [os.path.join(os.path.dirname(__file__), 'gsd-test-power')],

As I've mentioned above, you'll need to have a separate dconf database, otherwise it'll poke at the user's settings.

@@ +306,3 @@
+        self.set_power_setting('sleep-inactive-battery-timeout', 10)
+        # would be good to set this, but doesn't work ATM:
+        # https://bugzilla.gnome.org/show_bug.cgi?id=685947

Been fixed since.

@@ +313,3 @@
+        self.check_for_suspend(10)
+
+    def test_action_critical_battery(self):

For each test, I would use a separate file, to make it easier to just drop new files and get tests added.

@@ +323,3 @@
+        obj_bat.Set('org.freedesktop.UPower.Device', 'TimeToEmpty',
+                    dbus.Int64(30, variant_level=1),
+                    dbus_interface=dbus.PROPERTIES_IFACE)

Can we get helpers for those?

@@ +352,3 @@
+        # anything (as we still have the second battery)
+        time.sleep(25)
+        # check that it did not suspend or hibernate

Can we check for warnings?
That would mean reimplementing a notification daemon...

Maybe all those separate mock daemons should be in separate sources so we could reuse them for other parts. (I see that gvfs has polkit support).

(In reply to comment #0)
> - suspend on configured idle time (sleep-inactive-battery-timeout); this works
> on current master
> 
> - action on critical battery for one battery; works on current master
> 
> - (non-)action on critical battery for multiple batteries; currently fails; I
> don't want to exclude a bug in the test yet, but it's likely that this is an
> actual bug

As mentioned in the review, we could also test for:
- notifications being sent
- lid actions (XRandR mocking?)
- testing the exposed interface (used by gnome-shell right now)
- non-primary batteries

> On test failures it shows the daemon log. I'm using the
> plugins/power/gsd-test-power wrapper, as this can be called straight out of the
> build tree, and avoids a lot of chatting from the other plugins when reading
> the logs.

It also needs the power plugin to be disabled in gnome-settings-daemon, which isn't great if we don't override that.

> The actual test cases are reasonably small now, so it doesn't take much effort
> now to write them. It does take quite some effort to set up the test bed,
> though, I'm still trying to make that easier.

It's looking great already.

>  - It currently only supports ConsoleKit, I shall check config.h for
> HAVE_SYSTEMD and mock that instead. (This is only needed for gnome-session's
> idle handling and to pretend we are on a currently active session; we don't
> need any other API).

>  - It's currently one monolithic script; the GSDTestCase common code should
> eventually be split out as it'll be useful for other module tests as well (it
> starts a sandboxed gnome-session, etc)

I didn't really see it being sandboxed. Did I miss that part of the code?

> Before I spend more work on this, I'd like to discuss with you some questions:
> 
> (1) Are you interested in this at all? I'm happy to set up a
> chat/hangout/whatever with you to discuss why and how this would be beneficial,
> and what the longer-term plans are (wrt. jhbuild, OSTree, and the like)

This should be run on every distcheck, package compilation and buildbots, eventually.

> (2) I do all that D-Bus object mockery so that we can run the tests in a "make
> check" style without any particular privileges or assumptions about the
> environment. For example, this ought to work in "jhbuild build --check", even
> in a server-ish continuous integration environment. We can also support
> integration tests for VMs, then we can use the real system
> D-Bus/systemd/ConsoleKit; but we still need to mock upower so that we can forge
> various battery scenarios and the like. Are you okay with this unit test
> approach?

Sure.

> (3) Are you okay with the tests being written in Python (3)? It's the language
> I'm most efficient in, but if you don't want this as a test dependency, I can
> also try and write this in Vala or so (I'd just like to avoid plain C --
> keeping track of reference counting and memory allocations is just about the
> last thing I want to think about when writing tests)

Python is fine, as long as the test cases are easy to write.

Bastien, thanks for the review!

(In reply to comment #3)
> We shouldn't touch the user's setup or files. Reassign the XDG _DATA_DIRS/etc.
> to avoid this.

Agree, that's better.

> @@ +73,3 @@
> +
> +        klass.session_log = tempfile.NamedTemporaryFile()
> +        klass.session = subprocess.Popen(['gnome-session', '--session=null',
> '--debug'],
> 
> At the end of the day, I would rather we tested against a mock gnome-session by
> default, so as to avoid API change problems (as in, we're compiling a version
> that's newer than the one installed).

I'm a bit torn on this. I usually prefer testing against the real thing if possible (with system stuff such as upower or systemd it's unavoidable), as this is more realistic and also points out exactly that: g-s-d tests failing if the gnome-session API changed in jhbuild, so we know immediately if we have to fix stuff. My gut feeling is that it would be better to fail in the "API change case".

But in principle we can also mock it, of course. Would you agree that we can defer this to "version 2" of the tests?

> The ConsoleKit stuff would obviously have to get replaced or supplemented by
> the systemd support.

Obviously, I pointed that out in the original comment.

> +    '''Test the power plugin'''
> +
> +    def setUp(self):
> 
> Is this instantiated for each test?

Yes. setUp()/tearDown() run for each "def test_*", while setUpClass()/tearDownClass() run only once per unittest.TestCase class.

> As I've mentioned above, you'll need to have a separate dconf database,
> otherwise it'll poke at the user's settings.

I'd like to do that indeed. Right now there is quite some cleanup code to ensure that the settings get reset after running the tests, but I'd rather avoid that. I just couldn't figure out how to do this, but I'll talk to Ryan Lortie.

> For each test, I would use a separate file, to make it easier to just drop new
> files and get tests added.

I'm fine with splitting the test classes into different files (like the generic GSDTestCase helpers and PowerPluginTest), but splitting each individual test function is going to be some effort: If you split each individual test into a new class, then it's less obvious which test harness (setUp()) is being used, and you'd have a rather deep inheritance hierarchy. It's very unusual in Python to split functions into separate files.

> @@ +323,3 @@
> +        obj_bat.Set('org.freedesktop.UPower.Device', 'TimeToEmpty',
> +                    dbus.Int64(30, variant_level=1),
> +                    dbus_interface=dbus.PROPERTIES_IFACE)
> 
> Can we get helpers for those?

Yes, absolutely. I started this in python-dbusmock version 0.2 which introduces templates for factorizing out common code, and we can add more convenience methods later on. I'll change the patch to make use of the templates, but at the time of the first version those didn't exist yet.

> @@ +352,3 @@
> +        # anything (as we still have the second battery)
> +        time.sleep(25)
> +        # check that it did not suspend or hibernate
> 
> Can we check for warnings?
> That would mean reimplementing a notification daemon...

Yes, we can do that. This sounds like a common case again, and the D-BUS API hasn't changed in a long time, so I'll add a template for the notification daemon to dbusmock.

> Maybe all those separate mock daemons should be in separate sources so we could
> reuse them for other parts. (I see that gvfs has polkit support).

Right, that was the idea behind the dbus mock templates. The fake polkit is not in upstream gvfs yet (just in the Ubuntu package), but it is in e. g. udisks2 and also other projects like aptdaemon, a bad duplication. These were written before dbusmock, too.


(In reply to comment #4)
> As mentioned in the review, we could also test for:
> - notifications being sent

See above.

> - lid actions (XRandR mocking?)

This is actually part of my research TODO in this cycle (see https://blueprints.launchpad.net/ubuntu/+spec/qa-r-test-technology-research). I do not yet know how far we can sensibly mock XRandR devices, but I hope it'll work. Multi-monitor setups have been breaking too often in the past, it would be nice to be able to automate testing for those.

> >  - It's currently one monolithic script; the GSDTestCase common code should
> > eventually be split out as it'll be useful for other module tests as well (it
> > starts a sandboxed gnome-session, etc)
> 
> I didn't really see it being sandboxed. Did I miss that part of the code?

That was confusing. I meant it is running on a private D-BUS without starting real applications/components, so it runs well inside a running GNOME session, as well as in a minimal server-like environment without any D-BUS or GNOME stuff at all.

Thanks,

Martin

Created attachment 231863 [details] [review]
power: check environment for reducing action delay

Sorry for the long delay; I worked on a different project in the last couple of weeks. I now changed the tests to mock systemd, and use a private/temporary XDG_{CONFIG,DATA}_HOME and XDG_RUNTIME_DIR so that changes do not affect your real dconf database and files.

Since the last time, test_sleep_inactive_battery() fails now; I guess that's a consequence of the two patches of bug 685951, even setting idle-dim-time and sleep-display-battery in addition doesn't help. But I'll work on that today.

This patch provides a prerequisite for not going crazy on waiting when running the tests: Usually there is a 20 second delay between determining an action ("critical battery") and executing it, which gives the user some time for saving documents and the like. But I'd like to get rid of that when testing. This patch turns the "20" into a global variable (in GsdPowerManagerPrivate) and changes it to "1" if $GSD_NO_ACTION_DELAY is set. Is that acceptable?

(In reply to comment #6)
> Since the last time, test_sleep_inactive_battery() fails now; I guess that's a
> consequence of the two patches of bug 685951

Err, bug 668703, sorry.

Created attachment 231866 [details] [review]
WIP: Add tests for power plugin

This is the current version of the tests. On today's git heads of gnome-session and gnome-settings-daemon, three tests succeed: sleep-inactive-battery-timeout with and without screen blanking (sleep-display-battery), and suspending on critical battery. The fourth test currently fails: having multiple batteries, only one of which is critical; the power plugin still suspends in that case. I believe this to be a genuine bug.

Notes:

 * This assumes that "power: check environment for reducing action delay" gets applied, otherwise the "wait for suspend" timeouts need to be increased by 20 seconds.

 * This now requires the "xte" program (http://hoopajoo.net/projects/xautomation.html) to simulate user activity to reset the idle timer. If that dependency is a problem, I'm open to suggestions how else to inject fake events into X.org.

 * The test_sleep_inactive_battery_with_blank() test will actually blank your monitor. But tearDown() will bring it back up again.

 * This doesn't yet intercept the "critical battery" notification bubbles, so they will actually appear. I will work on adding a standard mock template to dbusmock for notification-daemon.

Review of attachment 231863 [details] [review]:

::: plugins/power/gsd-power-manager.c
@@ +4072,3 @@
                                                          "time-action");
+        /* for normal operations we use 20 seconds, but skip this for tests */
+        if (g_getenv ("GSD_NO_ACTION_DELAY") != NULL)

It could be much simpler than that, and done in the class_init() to ensure it's only ever called once.

Make the variable a global. And parse the value of GSD_NO_ACTION_DELAY for the length (obviously, you'd need to rename it).

Review of attachment 231866 [details] [review]:

Looks good.

The other 2 requests I would have:
- Integrate in "make check"
- Make it print a list of required modules if any are missing (either package names, or integrate the modules in jhbuild so that it's easy to get running).

::: plugins/power/test.py
@@ +85,3 @@
+            os.makedirs(d)
+        with open(os.path.join(d, 'null.session'), 'w') as f:
+            f.write('''[GNOME Session]

Can you ship that file with some other framework? Or even re-use an installed one?

Otherwise I'd rather it was separate from the test case code, and copied as needed.

Created attachment 231893 [details] [review]
power: check environment for reducing action delay

This changes the "action delay env" according to your review. I use a simple atoi() with clamping to positive numbers; please let me know if you think we need a more elaborate handling of values like "1foo" (where "foo" would just be ignored).

Created attachment 231894 [details] [review]
WIP: Add tests for power plugin

This updates the test for the changed $GSD_ACTION_DELAY variable/semantics according to the previous patch.

It now also uses dbusmock's notification-daemon template to avoid actually popping up "your battery is critically low" notifications, and instead asserts that the notification is sent and talks about low batteries. Note that this needs python-dbusmock 0.3 (I asked Matej to update the Fedora package, in the meantime you can get it with pip install or run out of trunk).

I'll respond to your review comments in a separate reply.

Review of attachment 231893 [details] [review]:

Looks good otherwise.

::: plugins/power/gsd-power-manager.c
@@ +3301,3 @@
+                /* use the default for invalid values */
+                if (critical_action_delay < 0) {
+                        g_warning ("Invalid value of $GSD_ACTION_DELAY, ignoring");

g_warning ("Invalid value '%s' for $GSD_ACTION_DELAY, ignoring", env_action_delay);

(In reply to comment #10)
> The other 2 requests I would have:
> - Integrate in "make check"

That's on my list, but as it is now I'm not quite happy with the structure. The common GSDTestCase class should be in a separate file; maybe in data/, or a new <root>/tests/ directory? The latter might be adequate if we also want to test the main daemon at some point, in addition to the plugins. GSDTestCase is general test infrastructure which will also be needed for the other tests (keyboard is on my TODO list, and let's see how far we get with xrandr).

Also, I wouldn't like to add it to "make check" while a test is failing. I could either mark it as exfail and file a bug about it, or we just fix it.

> - Make it print a list of required modules if any are missing (either package
> names, or integrate the modules in jhbuild so that it's easy to get running).

AFAICS, this needs:

 * dbus-python and python-dbusmock: It will fail hard with an ImportError stating the name if they are missing; I could change it to just exit with 0 with an error message; sounds ok?

 * The xte program: if not present, I could skip the whole test suite with an appropriate message.

With the ones from above, people can run jhbuild with --check in very limited environments without provoking a failure.

 * gnome-session is the only jhbuild module which we need; it could be from jhbuild, or from your system install. If it's not present anywhere, it could just skip the test similarly to xte.

This should provide the "print list of required stuff"; sounds ok?

> ::: plugins/power/test.py
> @@ +85,3 @@
> +            os.makedirs(d)
> +        with open(os.path.join(d, 'null.session'), 'w') as f:
> +            f.write('''[GNOME Session]
> 
> Can you ship that file with some other framework? Or even re-use an installed
> one?
> 
> Otherwise I'd rather it was separate from the test case code, and copied as
> needed.

Ah, matter of style I guess. I usually prefer to create such simple files inline over cluttering the source tree with such tiny files. That's part of the general GSDTestCase code. If that moves to <root>/tests/, would that suffice for you or do you still want them as permanent files in <root>/tests/ as well?

Thanks!

Martin

P.S. My last day for this year, I'll be back on January 4 to continue the discussion. Happy end-of-year holidays!

Comment on attachment 231893 [details] [review]
power: check environment for reducing action delay

Committed with the suggested change of showing the bad value.

(In reply to comment #14)
> (In reply to comment #10)
> > The other 2 requests I would have:
> > - Integrate in "make check"
> 
> That's on my list, but as it is now I'm not quite happy with the structure. The
> common GSDTestCase class should be in a separate file; maybe in data/, or a new
> <root>/tests/ directory? The latter might be adequate if we also want to test
> the main daemon at some point, in addition to the plugins. GSDTestCase is
> general test infrastructure which will also be needed for the other tests
> (keyboard is on my TODO list, and let's see how far we get with xrandr).

tests/ is fine by me.

> Also, I wouldn't like to add it to "make check" while a test is failing. I
> could either mark it as exfail and file a bug about it, or we just fix it.

I'd rather it was done, so that we could start fixing it ;)

> > - Make it print a list of required modules if any are missing (either package
> > names, or integrate the modules in jhbuild so that it's easy to get running).
> 
> AFAICS, this needs:
> 
>  * dbus-python and python-dbusmock: It will fail hard with an ImportError
> stating the name if they are missing; I could change it to just exit with 0
> with an error message; sounds ok?

Please.

>  * The xte program: if not present, I could skip the whole test suite with an
> appropriate message.

I forgot to mention that I'd rather you used XTest directly for that. We can even create a small program for it if it's not usable from Python (we use it in the wacom plugin for example).

> With the ones from above, people can run jhbuild with --check in very limited
> environments without provoking a failure.
> 
>  * gnome-session is the only jhbuild module which we need; it could be from
> jhbuild, or from your system install. If it's not present anywhere, it could
> just skip the test similarly to xte.
> 
> This should provide the "print list of required stuff"; sounds ok?

Sounds fine.

> > ::: plugins/power/test.py
> > @@ +85,3 @@
> > +            os.makedirs(d)
> > +        with open(os.path.join(d, 'null.session'), 'w') as f:
> > +            f.write('''[GNOME Session]
> > 
> > Can you ship that file with some other framework? Or even re-use an installed
> > one?
> > 
> > Otherwise I'd rather it was separate from the test case code, and copied as
> > needed.
> 
> Ah, matter of style I guess. I usually prefer to create such simple files
> inline over cluttering the source tree with such tiny files. That's part of the
> general GSDTestCase code. If that moves to <root>/tests/, would that suffice
> for you or do you still want them as permanent files in <root>/tests/ as well?

<root>/tests is better IMO.

> Thanks!
> 
> Martin
> 
> P.S. My last day for this year, I'll be back on January 4 to continue the
> discussion. Happy end-of-year holidays!

See you next year ;)

Created attachment 232770 [details] [review]
Add support for plugin tests

I believe I now have covered all the review comments. I split the stuff into two commits, one for adding the generic infrastructure and one for the power plugin test.

This is the "infrastructure" commit, providing the GSDTestCase class and the auxiliary files and tools in tests/. It does not add tests by itself. The commit changelog has the details.

Created attachment 232771 [details] [review]
Add tests for the power plugin

This adds the actual tests for the power plugin. I tested it with a normal "inline" build, with a separate build tree (mkdir b; cd b; ../configure; make check), and with "jhbuild run make distcheck". It still fails the "multiple batteries" test case, but otherwise runs well for me.

Please note that if you get a weird pygobject library error in jhbuild, you need to run jhbuild under PYTHON=python3; this is covered in bug 688353, but as current GNOME's goal is to port everything to python3 I wouldn't like to add new python2 code. I. e. :

  PYTHON=python3 jhbuild buildone --check gnome-settings-daemon

should build the full thing, succeed three tests, and fail in test_action_multiple_batteries with an unexpected suspend when the first battery goes critical, and show the power plugin log.

Created attachment 232894 [details] [review]
Add tests for the power plugin

Relax mock log parsing to also work with current python-dbusmock which now also puts the method call arguments into the log. With this it fails as expected again with detecting the unexpected hibernate after the first battery goes critically low.

Created attachment 232895 [details] [review]
Add tests for the power plugin

I went through the logic for the composite device, and noticed that it uses the Energy and EnergyFull properties instead of the Percentage properties. That's what caused the test failure; I didn't include these properties into the mock template originally, as upower's documentation says that these might not be present on some devices. python-dbusmock 0.3.1 now has an upower template which does set those, and with this updated test case (which updates "Energy" along with "TimeToEmpty") the "multiple batteries" test case now works fine as well.

Created attachment 232896 [details] [review]
Add support for plugin tests

As discussed on IRC, with this adjustment the GSDTestCase class works with both Python 2 and 3.

Created attachment 232897 [details] [review]
Add tests for the power plugin

Likewise, this adjusts the tests to also work with Pyton 2, and reorders the imports so that gsdtestcase's "missing module" error messages work for dbus and dbusmock.

To run this with python 2, merely change the hashbang to "env python".

Both committed with a few additional fixes from Martin. Good job!

Attachment 232896 [details] pushed as 2fbb404 - Add support for plugin tests
Attachment 232897 [details] pushed as ed68baf - Add tests for the power plugin