GNOME Bugzilla – Bug 708280
test_no_suspend_lid_close() is flawed
Last modified: 2018-01-17 17:37:31 UTC
The test checks that we don't suspend using the logind/upower API, but that doesn't reflect what happens at lid close in reality (suspension is initiated by logind, and we just inhibit/uninhibit as needed). We should check for active inhibitors instead. (Also, the mock inhibit API is kind of broken - by returning a low fake FD, a close() call can by accident close the X11 socket or the session bus)
Created attachment 366948 [details] [review] power: Correctly test lid related suspend inhibitions The test was trying to verify that no suspend action happened. However, this cannot happen during testing, as the suspend action for lid close is not done by gsd-power. So instead just verify that the lid switch inhibition works as expected.
I am not entirely sure what is going on with the mock FD passing exactly. However, the FD that gsd-power receives according to the log looks like a proper value. Is the hardcoded 3 not just the FD number on the mock side? So, I think it is working properly and I don't see a reason to dig deeper.
Review of attachment 366948 [details] [review]: Couple of nits, looks good to commit if this passes the tests. ::: plugins/power/test.py @@ +282,3 @@ self.fail('timed out waiting for logind Suspend() call') + def check_for_inhibited(self, timeout=0): Can you rename those to "lid_inhibited"... @@ +290,3 @@ + 'Timed out waiting for lid inhibitor') + + def check_for_uninhibited(self, timeout=0): and "lid_uninhibited"? It makes it clearer which inhibitor we're checking for. @@ +756,3 @@ self.set_has_external_monitor(False) + + # Check that no action happens during the safety time (-1 second) replace "safety time (-1 second)" with "safety time minus 1 second". I thought the safety was "-1 second".
(In reply to Benjamin Berg from comment #2) > I am not entirely sure what is going on with the mock FD passing exactly. > However, the FD that gsd-power receives according to the log looks like a > proper value. Is the hardcoded 3 not just the FD number on the mock side? > > So, I think it is working properly and I don't see a reason to dig deeper. Which value do you see printed on the gsd-power side? I have no idea what value it ultimately is, but if it's indeed 3, then this is something to fix on the dbusmock side.
(In reply to Bastien Nocera from comment #4) > (In reply to Benjamin Berg from comment #2) > > I am not entirely sure what is going on with the mock FD passing exactly. > > However, the FD that gsd-power receives according to the log looks like a > > proper value. Is the hardcoded 3 not just the FD number on the mock side? > > > > So, I think it is working properly and I don't see a reason to dig deeper. > > Which value do you see printed on the gsd-power side? I have no idea what > value it ultimately is, but if it's indeed 3, then this is something to fix > on the dbusmock side. First 14 then 15 or so. So really looks like a proper FD.
Thanks for the review! Attachment 366948 [details] pushed as 17562b1 - power: Correctly test lid related suspend inhibitions