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

unit tests for ansible.module_utils.urls #38059

Merged
merged 18 commits into from Apr 9, 2018
Merged

Conversation

sivel
Copy link
Member

@sivel sivel commented Mar 28, 2018

SUMMARY

unit tests for ansible.module_utils.urls

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

tests/units/module_utils/

ANSIBLE VERSION
devel
ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Mar 28, 2018

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Mar 28, 2018
Copy link
Member

@mattclay mattclay left a comment

Choose a reason for hiding this comment

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

Could you submit the ansible-test changes in a separate PR?

if args.include:
options.extend([
'--include',
args.include
Copy link
Member

Choose a reason for hiding this comment

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

Style recommendation....

Since this option (and the following one) only take a single argument, it would be easier to read as:

options.extend(['--include', args.include])

Keeping the option and the arg together is also more readable with multiple options (doesn't apply in this case):

options.extend([
    '--include', args.include,
    '--some-other-arg', args.some_other_arg,
])

@ansibot
Copy link
Contributor

ansibot commented Mar 28, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/module_utils/urls/test_urls.py:10:1: E265 block comment should start with '# '

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Mar 28, 2018
@sivel
Copy link
Member Author

sivel commented Mar 28, 2018

Could you submit the ansible-test changes in a separate PR?

Done. See #38061

I'll rebase this to remove the commit after the other is merged.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 28, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 28, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/module_utils/urls/test_open_url.py:43:1: E302 expected 2 blank lines, found 1

The test ansible-test sanity --test replace-urlopen [explain] failed with 17 errors:

test/units/module_utils/urls/test_open_url.py:15:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:19:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:44:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:48:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:63:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:71:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:74:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:80:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:101:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:120:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:126:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:129:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:150:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:157:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:162:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:168:12: use `ansible.module_utils.urls.open_url` instead of `urlopen`
test/units/module_utils/urls/test_open_url.py:174:5: use `ansible.module_utils.urls.open_url` instead of `urlopen`

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Mar 28, 2018
@sivel
Copy link
Member Author

sivel commented Mar 28, 2018

@mattclay ansible-test changes removed from this PR

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 28, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Mar 29, 2018
@sivel sivel force-pushed the urls-units branch 2 times, most recently from fcd8836 to 05e9565 Compare March 29, 2018 18:17
@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Mar 29, 2018
@sivel sivel changed the title [WIP] unit tests for ansible.module_utils.urls unit tests for ansible.module_utils.urls Mar 29, 2018
@ansibot ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Mar 29, 2018
@sivel
Copy link
Member Author

sivel commented Mar 29, 2018

Removing WIP. I'm going with this as is for now. These tests alone cover 52% of ansible.module_utils.urls.

ready_for_review

@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Mar 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 29, 2018

The test ansible-test sanity --test no-underscore-variable [explain] failed with 2 errors:

test/units/module_utils/urls/test_fetch_url.py:64:5: use `dummy` instead of `_` for a variable name
test/units/module_utils/urls/test_fetch_url.py:86:5: use `dummy` instead of `_` for a variable name

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 29, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 30, 2018
Copy link
Member

@mattclay mattclay left a comment

Choose a reason for hiding this comment

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

The tests look good. I'd just move/rename a few of the tests for consistency.

@@ -0,0 +1,22 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Rename this file to test_RequestWithMethod.py to match the class it is testing and give the test function test_RequestWithMethod a more specific name. Alternatively, move the test into test_urls.py instead.

@@ -0,0 +1,66 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Rename this file to generic_urlparse.py to match the function being tested.

from ansible.module_utils.six.moves.urllib.parse import urlparse, urlunparse


def test_ParseResultDottedDict():
Copy link
Member

Choose a reason for hiding this comment

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

Move this test function into a new test_ParseResultDottedDict.py file to match the class it is testing and give the test function test_ParseResultDottedDict a more specific name. Alternatively, move the test into test_urls.py instead.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 2, 2018
@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/module_utils/urls/test_RedirectHandlerFactory.py:90:161: E501 line too long (161 > 160 characters)

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 6, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 6, 2018
…ror_307 does not cause issues with urllib2 type redirects
@sivel
Copy link
Member Author

sivel commented Apr 9, 2018

A few unstable results in CI. Not related. Merging.

@sivel sivel merged commit 6332bee into ansible:devel Apr 9, 2018
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
* Start of tests for ansible.module_utils.urls

* Start adding file for generic functions throughout urls

* Add tests for maybe_add_ssl_handler

* Remove commented out line

* Improve coverage of maybe_add_ssl_handler, test basic_auth_header

* Start tests for open_url

* pep8 and ignore urlopen in test_url_open.py tests

* Extend auth tests, add test for validate_certs=False

* Finish tests for open_url

* Add tests for fetch_url

* Add fetch_url tests to replace-urlopen ignore

* dummy instead of _

* Add BadStatusLine test

* Reorganize/rename tests

* Add tests for RedirectHandlerFactory

* Add POST test to confirm behavior is to convert to GET

* Update tests to handle recent changes to RedirectHandlerFactory

* Special test, just to confirm that aliasing http_error_308 to http_error_307 does not cause issues with urllib2 type redirects
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants