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

Added tests for Hoppy #26

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Added tests for Hoppy #26

wants to merge 1 commit into from

Conversation

DoomTaper
Copy link
Contributor

No description provided.

@DoomTaper DoomTaper force-pushed the develop branch 2 times, most recently from 70fa770 to 7df78f7 Compare February 5, 2017 20:01
Copy link
Contributor

@DePierre DePierre left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Could you look at the comments and improve your PR?

try:
response = self._re_response.search(transaction).group().strip() + '\n\n'
except AttributeError:
response = "NOT FOUND"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it would be better to have a sentinel value like None or '' instead of a hardcoded arbitrary string.

response = self._re_response.search(transaction).group().strip() + '\n\n'
except AttributeError:
response = "NOT FOUND"
if(response != "NOT FOUND"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Superfluous parenthesis.

# HoppyParser.is_mine
###
def test_parser_hoppy_is_mine(self):
self.assertTrue(HoppyParser.is_mine('./'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you apply what the other tests are doing? Meaning to save the report in a python file and import it here. Also mock the underlying function that will read the report (see https://github.com/owtf/ptp/blob/develop/tests/tools/arachni/test_parser.py for instance).

Using relative directory ./ could introduce some bugs that would be annoying to find and fix.


from ptp.tools.hoppy.parser import HoppyParser

class TestHoppyParser(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you improve the tests for Hoppy? Add tests to check that it doesn't have false-positive when checking reports from other tools, etc.

See https://github.com/owtf/ptp/blob/develop/tests/tools/wapiti/test_parser.py for example.

HoppyParser.__format__ = ''
my_hoppy = HoppyParser("./")
report = my_hoppy.parse_report()
assert_that(4, equal_to(len(report[-1]['transactions'])))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very weak. Could you improve on it? And verify that each element of transactions is what you expect as well? It would be easy to imagine a bug in the regexes where 4 elements would match but each element would be garbage.

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.

None yet

2 participants