Skip to content

Tests(Firehol): Adding and improving tests for Firehol.#697

Merged
regulartim merged 4 commits intointelowlproject:developfrom
amishhaa:fix-test-fh
Jan 13, 2026
Merged

Tests(Firehol): Adding and improving tests for Firehol.#697
regulartim merged 4 commits intointelowlproject:developfrom
amishhaa:fix-test-fh

Conversation

@amishhaa
Copy link
Copy Markdown
Contributor

@amishhaa amishhaa commented Jan 11, 2026

Description

This PR fixes and adds new test cases for test_firehol.py.
Previously .status_code was incorrectly used and we should be using .raise_for_status instead to mock the behavior of the status code. Additionally we can also test the method which cleans up old entries, and test edges like failure from certain network codes.

Related issues

Fixes #694.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • I have read and understood the rules about how to Contribute to this project.
  • The pull request is for the branch develop.
  • I have added documentation of the new features.
  • [xx] Linters (Black, Flake, Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
  • I have added tests for the feature/bug I solved. All the tests (new and old ones) gave 0 errors.
  • If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).
  • If the GUI has been modified:
    • I have a provided a screenshot of the result in the PR.
    • I have created new frontend tests for the new component or updated existing ones.

Important Rules

  • If you miss to compile the Checklist properly, your PR won't be reviewed by the maintainers.
  • If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review. After being reviewed and received a "change request", you should explicitly ask for a review again once you have made the requested changes.

@amishhaa
Copy link
Copy Markdown
Contributor Author

amishhaa commented Jan 11, 2026

@regulartim, please check this PR out :)!

@amishhaa amishhaa mentioned this pull request Jan 11, 2026
Closed
regulartim
regulartim requested changes Jan 11, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@regulartim regulartim left a comment

Choose a reason for hiding this comment

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

I like it! Could you also add a test case for a list containing an invalid IP address?

@amishhaa
Copy link
Copy Markdown
Contributor Author

amishhaa commented Jan 12, 2026

@regulartim regarding adding a test for invalid ip, from following the function call hierarchy where these requests are actually processed from firehol.py, firehol.py uses FireHolList from models.py, i was not able to find any place where invalid IPs are pre-processed/validated, and if we add tests here for invalidating ips, we might also need to do it from the root level in the FireHolList data structure in models.py which is actually responsible for invalidating ips but no exceptions are raised there basically everything is valid currently so we cannot test it. I was also not able to find any unit tests for it in test_models.py, please let me know what you think, because in my opinion is something like invalidating ip is required it would have to be implemented in a fresh pr because from my knowledge it doesnt exist currently.

@regulartim
Copy link
Copy Markdown
Collaborator

regulartim commented Jan 13, 2026

@regulartim regarding adding a test for invalid ip, from following the function call hierarchy where these requests are actually processed from firehol.py, firehol.py uses FireHolList from models.py, i was not able to find any place where invalid IPs are pre-processed/validated, and if we add tests here for invalidating ips, we might also need to do it from the root level in the FireHolList data structure in models.py which is actually responsible for invalidating ips but no exceptions are raised there basically everything is valid currently so we cannot test it. I was also not able to find any unit tests for it in test_models.py, please let me know what you think, because in my opinion is something like invalidating ip is required it would have to be implemented in a fresh pr because from my knowledge it doesnt exist currently.

Sorry, my bad. I confused the FireHolCron with MassScannersCron, where we actually have validation. So, you don't need to add tests for this. However I do think we should implement some form of validation, maybe the same process we have in MassScannersCron. Would you like to open an issue for that?

@amishhaa
Copy link
Copy Markdown
Contributor Author

amishhaa commented Jan 13, 2026 •
edited
Loading

Would you like to open an issue for that?

@regulartim Yes absolutely, i think we can wait for this to get resolved then the validation can be added on top with the tests that are required for it, i will open an issue and mention this PR as a blocker. let me know if you would like if it is done differently, like adding the validations first and then the tests can be pushed at once, or everything can be done in this pr itself.

@amishhaa amishhaa mentioned this pull request Jan 13, 2026
Closed
@regulartim
Copy link
Copy Markdown
Collaborator

regulartim commented Jan 13, 2026

Thanks for opening the issue #702 . I would suggest to finish this PR independent of the issue. The person who solves #702 can then add the corresponding tests.

@amishhaa
Copy link
Copy Markdown
Contributor Author

amishhaa commented Jan 13, 2026

I would suggest to finish this PR independent of the issue.

okay got it :)

@regulartim regulartim merged commit 7fb5a99 into intelowlproject:develop Jan 13, 2026
5 checks passed
@amishhaa amishhaa deleted the fix-test-fh branch January 13, 2026 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@regulartim regulartim regulartim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amishhaa @regulartim