Queues tests #551

bebehei · 2018-10-01T20:39:48Z

Edit: Fixes #563

bebehei · 2018-11-15T11:24:27Z

@tsipinakis There's only one question, which bothers me and is independent of #502. could you please help me with this commit?

Ignore test(temporarily to get build results)

If I'm passing a notification with the ID 1000 and there hadn't been a notification with 1000 before, why does the notification have to get a new ID? This hit me quite unexpected. I expected to actually use 1000 as the ID.

I don't remember it why, but I remember we had a discussion about that. Do you have any reasons to or the actual discussion?

tsipinakis · 2018-11-15T11:36:04Z

That's a very good catch! We hadn't had a discussion on it but previously we assigned the ID asked anyway. The notification spec doesn't specify what to do if the ID is invalid but we should preserve the previous behaviour to not break those that use a static id replacement as the old workaround for stack_tag.

I consider this a bug and it was probably introduced in #552.

tsipinakis · 2018-11-15T11:58:27Z

I pushed a fix @bebehei

tsipinakis

I also found a bug: After a notification is shown and closed the dunst window isn't hidden, a small black rectangle keeps being displayed (and moves around the screens with the cursor as would be expected). Bisect showed 052d9f7 as the broken commit.

test/queues.h

src/dunst.c

src/queues.c

test/queues.c

bebehei · 2018-11-18T23:32:51Z

Most of the stuff is now fixed.

I also found a bug: After a notification is shown and closed the dunst window isn't hidden, a small black rectangle keeps being displayed (and moves around the screens with the cursor as would be expected). Bisect showed 052d9f7 as the broken commit.

This is now fixed. The visible field of the dunst_status structure was absolutely redundant. I tripped too much over the word visible, which had different meanings in different contexts.

But shame on me, that I requested your review, but did not even bother to run dunst 🤕

bebehei · 2018-11-24T14:19:30Z

Heads up: I pushed a fixup for a small bug in my id assignment commit.

Thoughts in order:

Yay, thanks!
Wait, all your comments which got automatically marked outdated, aren't outdated anymore!?
Surprise surprise, all my changes are gone! 🎉

I'm glad that I'm a GitHub beta member (you can click on the images to see the diff):

Please fetch the upstream pull request the next time first. If you need to fetch them very easily, there's a simple refspec available to fetch the GitHub PRs automatically: https://bebehei.de/github-prs/

In previous releases when a replace request came in with an id that doesn't exist we created a new notification with it anyway. This is used by some to imitate the behaviour of `stack_tag` and while not recommended (as it will break if another notification gets assigned that id) we want to avoid such subtle breakages without consideration. This bug was introduced in d879d70.

When using a format with a trailing % character, dunst ends in an endless loop, searching for a % char, while pointing exactly with the haystack on the % character. Increasing the substring pointer will shift the pointer forwards onto the actual NULL character and stop the loop.

Only if the queue is full, dunst should swap the most important notifications from waiting with the least important notifications from displayed.

Notifications listed in the waiting queue are never shown :see_no_evil:

These aren't used anymore. Any notification which gets moved from waiting to displayed, will have set the start field with the current monotonic time. So testing start == 0 won't ever succeed in queues_notification_is_finished, as the tested notification is contained in the displayed queue. So the start field never will be 0. Also there's no semantics for start being 0 in dunst actually implemented.

tsipinakis · 2018-11-24T14:50:14Z

Surprise surprise, all my changes are gone! tada

Oh crap, sorry! Totally forgot to pull your changes before rebasing!

bebehei · 2018-11-24T15:42:28Z

Well, this happens.

Would you mind giving a final review?

tsipinakis · 2018-11-26T09:46:45Z

test/queues.c

+
+        n1 = test_notification("n1", 1);
+        n2 = test_notification("n2", 1);
+        n3 = test_notification("n3", 1);


Looking at this again the timeout parameter does seem redundant. What's the point in setting it to 1 here? Any value wouldn't make a difference as we aren't advancing time.

But anyway, that's a complete nitpick everything else looks good.

I guess this is a typo introduced via vim block mode during refactoring.

bebehei force-pushed the queues-tests branch 4 times, most recently from a8b71d0 to f524df3 Compare October 2, 2018 15:17

bebehei force-pushed the queues-tests branch 2 times, most recently from 8c7510f to 88aae62 Compare October 11, 2018 00:16

bebehei added this to the v1.4.0 milestone Oct 11, 2018

bebehei force-pushed the queues-tests branch from a1c7742 to 443de14 Compare October 11, 2018 15:03

bebehei mentioned this pull request Nov 6, 2018

Moving tests to GLib? #473

Closed

bebehei force-pushed the queues-tests branch 5 times, most recently from b548680 to e66c1fa Compare November 14, 2018 17:50

bebehei mentioned this pull request Nov 15, 2018

Coveragereport and test suite improvements #502

Merged

bebehei force-pushed the queues-tests branch 3 times, most recently from 6a1f40a to 7258b33 Compare November 15, 2018 11:13

bebehei changed the title ~~[WIP] Queues tests~~ Queues tests Nov 15, 2018

bebehei force-pushed the queues-tests branch 2 times, most recently from bd77815 to 893a92b Compare November 15, 2018 14:26

bebehei requested a review from tsipinakis November 15, 2018 15:16

tsipinakis requested changes Nov 18, 2018

View reviewed changes

bebehei force-pushed the queues-tests branch from 2545960 to 7b10185 Compare November 18, 2018 22:24

bebehei force-pushed the queues-tests branch from 7b10185 to 7946994 Compare November 18, 2018 23:33

bebehei requested a review from tsipinakis November 18, 2018 23:38

bebehei mentioned this pull request Nov 24, 2018

Dunst doesn't respect -r (replace) anymore #563

Closed

tsipinakis and others added 22 commits November 24, 2018 15:22

Add Framework for queues tests

e04003e

Add tests for queue teardown

fb5926b

Test insertion mechanisms

294cfdf

Test notification closing in queues

ef08f6a

Introduce a status structure

140278a

Test sleep times for get_next_datachange

662b22e

Test queue length reporting functions

5cf9e51

Add queue history tests

f1e3916

Test stacking

e6e0d88

Add tests for checking notifications timing out

2c1de2d

Add tests for fullscreen behavior

3fbd1fc

Add test for pausing behavior

acee260

Add tests for seeping important notifications

24e0e25

Add tests for xmore layout behavior

3e0daf4

Add faulty notification seep test

0b7a31b

Fix notification seeping when displayed isn't full

475d829

Only if the queue is full, dunst should swap the most important notifications from waiting with the least important notifications from displayed.

Do not mark the waiting notification as shown

9878de3

Notifications listed in the waiting queue are never shown :see_no_evil:

Merge queue_update and queue_check_timeouts

711b11c

Move field definition on top of function (readability)

d96a29f

bebehei force-pushed the queues-tests branch from 811090d to 93f6eb5 Compare November 24, 2018 14:25

tsipinakis approved these changes Nov 26, 2018

View reviewed changes

bebehei merged commit 7ba6ca9 into dunst-project:master Nov 26, 2018

bebehei deleted the queues-tests branch November 26, 2018 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queues tests #551

Queues tests #551

bebehei commented Oct 1, 2018 •

edited

bebehei commented Nov 15, 2018

tsipinakis commented Nov 15, 2018

tsipinakis commented Nov 15, 2018

tsipinakis left a comment

bebehei commented Nov 18, 2018

bebehei commented Nov 24, 2018

tsipinakis commented Nov 24, 2018 •

edited

bebehei commented Nov 24, 2018

tsipinakis Nov 26, 2018

bebehei Nov 26, 2018

Queues tests #551

Queues tests #551

Conversation

bebehei commented Oct 1, 2018 • edited

bebehei commented Nov 15, 2018

tsipinakis commented Nov 15, 2018

tsipinakis commented Nov 15, 2018

tsipinakis left a comment

Choose a reason for hiding this comment

bebehei commented Nov 18, 2018

bebehei commented Nov 24, 2018

tsipinakis commented Nov 24, 2018 • edited

bebehei commented Nov 24, 2018

tsipinakis Nov 26, 2018

Choose a reason for hiding this comment

bebehei Nov 26, 2018

Choose a reason for hiding this comment

bebehei commented Oct 1, 2018 •

edited

tsipinakis commented Nov 24, 2018 •

edited