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

Edge case testing, boundary testing, negative testing http analyzer #516

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

zeyadtmi
Copy link
Contributor

@zeyadtmi zeyadtmi commented Apr 4, 2024

Fixes Issue

Closes #515

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

@zeyadtmi zeyadtmi changed the base branch from master to develop April 4, 2024 18:10
@AlyaGomaa
Copy link
Collaborator

Hello @zeyadtmi Thanks for your PR, i'm currently reviewing it

However i have one small comment

When you open 2 new PRs, each PR should be separated and based on the origin/develop instead of your local develop or any other branch

I see what you did here is, made the first threat_intel PR in a branch, opened a PR, then on the same branch, you added the http tests and then opened another PR

The issue in what you did is, if your first PR failed, your second PR will fail too because the changes you did in the first one is also there in the second one

@AlyaGomaa
Copy link
Collaborator

Also the tests/test_http_analyzer.py::test_check_suspicious_user_agents_parametrized test failed. so i suggest you always test your PR locally before opening a PR in any repo.

I will re-review your PR once you fix it and post a screenshot of all tests passing locally

Thanks for your effort!

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

Successfully merging this pull request may close these issues.

Improvement Suggestion for HTTP Analyzer Testing
2 participants