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

Add blacklisted reason field to alert sender #2670

Conversation

johannaengland
Copy link
Contributor

@johannaengland johannaengland commented Sep 1, 2023

Fixes #2664.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Test results

     12 files       12 suites   15m 44s ⏱️
3 190 tests 3 190 ✔️ 0 💤 0
9 045 runs  9 045 ✔️ 0 💤 0

Results for commit b4960c8.

♻️ This comment has been updated with latest results.

@johannaengland
Copy link
Contributor Author

Okay, by factoring out alert sender as a fixture I was able to fix some problems, but the tests are still not green. For some reason caplog doesn't capture the logging part, even though I am certain that it detects that the alert sender is blacklisted. I have verified that by changing the return value there and then checking for that. So if you have any ideas @lunkwill42 on why caplog is not doing what I expect it to be doing, please let me know.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to my caplog feedback, I see that many of your tests and fixtures lack a dependency to the db fixture. This means two things:

  1. You're not guaranteed to have a database service available when the fixture or test is run. (although, in the current state of the integration test suite, PostgreSQL will always be available when the tests run, just because your tests are in the integration directory).

  2. Although fixtures that add database data and removes it again on teardown is fine, using the db fixture will ensure that each test runs inside a transaction that is rolled back as soon as the test ends.

tests/integration/models/alert_test.py Outdated Show resolved Hide resolved
tests/integration/models/alert_test.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #2670 (b4960c8) into master (e2b92de) will increase coverage by 0.19%.
Report is 20 commits behind head on master.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #2670      +/-   ##
==========================================
+ Coverage   54.74%   54.94%   +0.19%     
==========================================
  Files         560      561       +1     
  Lines       40732    40828      +96     
==========================================
+ Hits        22298    22432     +134     
+ Misses      18434    18396      -38     
Files Changed Coverage Δ
python/nav/models/profiles.py 73.92% <72.00%> (+4.39%) ⬆️
python/nav/alertengine/base.py 25.49% <80.00%> (+1.10%) ⬆️
python/nav/alertengine/dispatchers/__init__.py 51.21% <100.00%> (+5.06%) ⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lunkwill42 lunkwill42 closed this Sep 7, 2023
@lunkwill42 lunkwill42 force-pushed the alertengine/add-blacklisted-reason-to-alert-sender branch from b4960c8 to e2b92de Compare September 7, 2023 09:17
@sonarcloud
Copy link

sonarcloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@lunkwill42
Copy link
Member

Since @johannaengland is out sick today, I decided to rebase and force-push this to get it ready for a merge. However, GitHub seems to have decided that my force-push constituted a reset to master, that the PR now contains no commits, and closed it.

Now I am no longer allowed to push back to Johanna's branch, which means I'm forced to create a new PR 😛

@hmpf
Copy link
Contributor

hmpf commented Sep 7, 2023

That's one of many reasons when a PR is approved I prefer to do the merge locally and push to master directly. A lot more WYSIWYG!

@johannaengland johannaengland deleted the alertengine/add-blacklisted-reason-to-alert-sender branch September 12, 2023 11:07
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.

Add blacklisted reason field to AlertSender
3 participants