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

Test coverage increased #188

Merged
merged 4 commits into from
Mar 14, 2019
Merged

Test coverage increased #188

merged 4 commits into from
Mar 14, 2019

Conversation

rjt-gupta
Copy link
Contributor

@rjt-gupta rjt-gupta commented Mar 13, 2019

Changes this PR proposes -

  1. Bumped test coverage of snare_helpers.py to 100%. Overall coverage increased to 75%.
  2. .coverage is added to .gitignore file.

Refs - #187


from snare.utils.snare_helpers import parse_timeout, str_to_bool


Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove extra line

self.v = None

def test_parse_timeout(self):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.


assert parse_timeout('24Y') == 86400 # Default 24H format is used.

def test_str_to_bool(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be put separately.

snare/cloner.py Outdated
@@ -19,7 +19,7 @@ def __init__(self, root, max_depth, css_validate):
self.max_depth = max_depth
self.moved_root = None
if len(self.root.host) < 4:
sys.exit('invalid taget {}'.format(self.root.host))
sys.exit('invalid target {}'.format(self.root.host))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make the PR focus on a certain task, please put it in a different PR.

@@ -63,12 +63,12 @@ def add_meta_tag(page_dir, index_page, config):
main_page = main.read()
soup = BeautifulSoup(main_page, 'html.parser')

if (google_content and soup.find("meta", attrs={"name": "google-site-verification"}) is None):
if google_content and soup.find("meta", attrs={"name": "google-site-verification"}) is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put it in a different PR and the rest too.

google_meta = soup.new_tag('meta')
google_meta.attrs['name'] = 'google-site-verification'
google_meta.attrs['content'] = google_content
soup.head.append(google_meta)
if (bing_content and soup.find("meta", attrs={"name": "msvalidate.01"}) is None):
if bing_content and soup.find("meta", attrs={"name": "msvalidate.01"}) is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@rjt-gupta
Copy link
Contributor Author

Okayy, Will do the changes asap!

@rjt-gupta
Copy link
Contributor Author

Updated the requested changes.

@@ -28,5 +28,8 @@ def test_add_meta_tag(self):
assert(soup.find("meta", attrs={"name": "google-site-verification"}) and
soup.find("meta", attrs={"name": "msvalidate.01"}))

config['WEB-TOOLS'] = dict(google='', bing='')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually, different input conditions for a method are tested using separate test methods, so you can have a method name like test_add_meta_tag_with_empty_tags or anything else.

def setUp(self):
self.v = None

def test_str_to_bool(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for this case. True, False and Error separate.

class TestParseTimeout(unittest.TestCase):

def test_parse_timeout(self):
assert parse_timeout('20H') == 72000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you write it as 20*60*60? It will make more sense. And update the others too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, makes more sense. Will do the changes

assert str_to_bool(self.v) is False

with self.assertRaises(ArgumentTypeError):
str_to_bool('twz')
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are using self.v for the previous two cases, do the same for this too.

@rjt-gupta
Copy link
Contributor Author

Changes Done.

def test_parse_timeout(self):
assert parse_timeout('20H') == 20*60*60
assert parse_timeout('10M') == 10*60
assert parse_timeout('1D') == 1*86400
Copy link
Collaborator

Choose a reason for hiding this comment

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

24*60*60

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry about this.

@rnehra01 rnehra01 merged commit c049841 into mushorg:master Mar 14, 2019
@rjt-gupta rjt-gupta deleted the test-coverage branch April 9, 2019 15:02
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.

2 participants