Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert that notification icon is not null in tests #1269

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

bynect
Copy link
Member

@bynect bynect commented Jan 25, 2024

This is a temporary fix to #1228 until we figure out what causes the issue in first place (icons not being loaded).

This simply adds assertion so that if the icon is null the test suite will not segfault

The ci will fail...

@bynect bynect force-pushed the tmpfix-segfault-test branch from 417dae3 to a3bfee8 Compare January 25, 2024 16:13
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2fcea84) 65.45% compared to head (417dae3) 65.47%.

❗ Current head 417dae3 differs from pull request most recent head a3bfee8. Consider uploading reports for the commit a3bfee8 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   65.45%   65.47%   +0.02%     
==========================================
  Files          46       46              
  Lines        7749     7754       +5     
==========================================
+ Hits         5072     5077       +5     
  Misses       2677     2677              
Flag Coverage Δ
unittests 65.47% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bynect
Copy link
Member Author

bynect commented Jan 25, 2024

The ci actually failed with another error unrelated to the changes and is not reproducible locally...
quite the mystery

@fwsmit
Copy link
Member

fwsmit commented Jan 30, 2024

I think the memory leak was addressed by @zappolowski in #1274

@fwsmit
Copy link
Member

fwsmit commented Jan 30, 2024

This change seems fine to me. Although the tests will probably not pass locally for you now then?

@fwsmit fwsmit merged commit 46f1f06 into dunst-project:master Jan 30, 2024
4 of 18 checks passed
@bynect
Copy link
Member Author

bynect commented Jan 31, 2024

This change seems fine to me. Although the tests will probably not pass locally for you now then?

Yes they fail but I still don't know why... but at least no segfault

@apprehensions
Copy link
Contributor

It is a better idea to skip the test if the icon is missing.

@bynect
Copy link
Member Author

bynect commented Feb 21, 2024

It is a better idea to skip the test if the icon is missing.

do you know how to do that? I am not an expert of greatest

@fwsmit
Copy link
Member

fwsmit commented Feb 21, 2024

It should be SKIP()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants