Closed tobast closed 1 year ago
One test does not pass: test_datachange_ttl
. The reason is that this test uses a timestamp that is not time_monotonic_now
for simplicity, but queues.c:506 calls time_monotonic_now
, and messes up n->start
wrt. the timestamp as seen by the test.
One test does not pass:
test_datachange_ttl
. The reason is that this test uses a timestamp that is nottime_monotonic_now
for simplicity, but queues.c:506 callstime_monotonic_now
, and messes upn->start
wrt. the timestamp as seen by the test.
Update: it seems fine to use the time
variable at those locations, so I changed it. The test suite now fully passes.
It still fails the the test_datachange_agethreshold_at_second
test
Indeed, I was too hasty. I'm checking why.
EDIT: Ah, this is because c25d91684db606d23c7d146dbae455eb5644e0e0 changed slightly the semantics of next_datachange. I probably did not re-run the testsuite after this one. I'm fixing the test.
@fwsmit The test is fixed in the latest revision
Merging #1158 (15b0941) into master (464076d) will increase coverage by
0.01%
. The diff coverage is84.61%
.
:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@ Coverage Diff @@
## master #1158 +/- ##
==========================================
+ Coverage 66.03% 66.04% +0.01%
==========================================
Files 46 46
Lines 7595 7601 +6
==========================================
+ Hits 5015 5020 +5
- Misses 2580 2581 +1
Flag | Coverage Δ | |
---|---|---|
unittests | 66.04% <84.61%> (+0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
src/notification.c | 60.47% <0.00%> (-0.15%) |
:arrow_down: |
src/queues.c | 92.06% <100.00%> (+0.05%) |
:arrow_up: |
test/queues.c | 98.97% <100.00%> (+<0.01%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
With the understanding of https://github.com/dunst-project/dunst/issues/1102#issuecomment-1511022163, I want to suggest to split this PR up in 2 parts. One part being fixing the mixup between n->start and n->timestamp. The other part being the feature to be a bit more lenient in the required sleep duration. I want to release the first part in the next point release to fix the cpu usage issues and the other part can wait till a next version.
Are you willing to split up this PR? It should be as easy as submitting the right commits
I just checked, and it seems to be totally possible. The first two commits (5525e99, cd65209) should go together in the point release, as the second one ensures that the tests are still passing; the third one (15b0941) can go in later (this is the one that tries to be clever about turns of second).
I'll split the PR very soon.
This third commit is dropped from this PR in the current revision. The new PR #1167 covers it.
Thanks, I'm merging this now. It would be good to add a few tests to prevent issues like this and similar in the future, but I'll be making that a separate issue
Related to issue #1102. Tested hastily, seems to fix the high CPU load.