Fix disabling notification timeout #3626
Conversation
When creating a notification with a timeout value of `0`, no timer is created. The same behaviour was intended when setting the timeout to `0` later, but it was never implemented. Fixes awesomeWM#3625.
The closure for the `die` method didn't need anything from the `set_timeout`, so it makes no sense to re-create the method every time the timeout is changed. It's sufficient to make it a regular method.
Codecov Report
@@ Coverage Diff @@
## master #3626 +/- ##
=======================================
Coverage 90.64% 90.65%
=======================================
Files 854 855 +1
Lines 54772 54790 +18
=======================================
+ Hits 49650 49668 +18
Misses 5122 5122
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks for this. Just a minor comment. Also adding a regression test might be relevant here. The notification module seems to have many of these papercut fixes over time, so the risk of regression is rather high. Maybe a new integration test module would make sense. The current test module is getting very large and complex.
|
|
||
| -- Wait for the notification's timeout to pass. | ||
| table.insert(steps, function() | ||
| os.execute("sleep 2") |
There was a problem hiding this comment.
Instead of sleeping 2 full second, can it usleep for 0.3s, check, then return nil if the condition isn't met? I have been bitten by hardcoded blocking sleep in tests before
There was a problem hiding this comment.
Not really. Technically, the test is for "will this notification exist indefinitely". And "infinity has passed" is not really a condition I can check for. The closes I have is "we're still there after the previous timeout value", but I need to know that said timeout has passed for that.
I guess I could try to add up elapsed time, but I don't have enough experience with the test framework's weird "emulate async by repeatedly calling the same step" to know if that's any better of an idea than sleeping for multiple seconds.
There was a problem hiding this comment.
Our test handle this internally. If something returns nil too many time, the test fails. This is transparent. Just check for the condition and return nil if it's not met, the framework contains the boilerplate and timeout/retry logic.
There was a problem hiding this comment.
As I said, I cannot "check for the condition". The condition is "infinity has passed and the notification is still visible".
The closest approximation is
- I know that if
.timeout = 0wasn't applied correctly, the timeout would likely still be 1 second - I then wait for a value greater than that known timeout to rule out race conditions if we waited for the same value, and check if the notification has expired
So I'll rephrase my second paragraph above:
Would it be acceptable to sum up the wait time, relying on the implementation detail that a sleep timer of 0.3 seconds and the fact that a step is "re-tried" 10 times ends up working out so that the total wait time in this case happens to be below the point where the test framework would bail?
I could also access the notification's internal values to check timer == nil, but that wouldn't be an integration test.
When creating a notification with a timeout value of
0, no timer iscreated. The same behaviour was intended when setting the timeout to
0later, but it was never implemented.
Fixes #3625.