Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[RFR] Parametrize Tower tests on API version #9888

Merged

Conversation

nachandr
Copy link
Contributor

@nachandr nachandr commented Jan 28, 2020

Purpose or Intent

PRT Run

{{ pytest: cfme/tests/services/test_config_provider_servicecatalogs.py cfme/tests/services/test_ansible_workflow_servicecatalogs.py --use-provider complete}}

@dajoRH dajoRH added the lint-ok label Jan 28, 2020
@nachandr nachandr force-pushed the parametrize_tower_tests_on_api_version branch from 75299a6 to 7efa1b5 Compare January 28, 2020 22:31

@pytest.fixture
def ansible_api_version_change(provider, ansible_api_version):
"""Fixture to update Tower url to /api/v2 so that API v2 can be tested.
Copy link
Member

Choose a reason for hiding this comment

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

👌 Thanks for the thorough docblock!


if ansible_api_version == 'v2':
updated_url = f'{parsed.scheme}://{parsed.netloc}/api/{ansible_api_version}'
original_url = provider.url
Copy link
Member

Choose a reason for hiding this comment

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

Looking good, but I would do two things here:

  1. move this assignment of original_url outside of the logic blocks, so that we don't hit any UnboundLocals
  2. Make the url update regardless of ansible_api_version, so that its always explicitly set to what the test parameter is.

If we're not explicitly setting it, we risk running into situations where fixture/parameter scope, or the URL defined for the provider in the yaml, will result in the provider's configuration not matching the parametrized value for API.

This way we can remove the logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@mshriver mshriver left a comment

Choose a reason for hiding this comment

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

Two comments to consider - nice work though!

@mshriver
Copy link
Member

PRT failures look unrelated to the changes here - other than MIQ's lack of support for v2 API.

@john-dupuy john-dupuy changed the title [RFR] Parametrize Tower tests on API version [WIPTEST] Parametrize Tower tests on API version Jan 30, 2020
@nachandr nachandr changed the title [WIPTEST] Parametrize Tower tests on API version [RFR] Parametrize Tower tests on API version Jan 31, 2020
@dajoRH
Copy link
Contributor

dajoRH commented Jan 31, 2020

I detected some fixture changes in commit 445388f

The global fixture ansible_api_version_change is used in the following files:

  • cfme/tests/services/test_ansible_workflow_servicecatalogs.py
  • cfme/tests/services/test_config_provider_servicecatalogs.py

The local fixture ansible_workflow_catitem is used in the following files:

  • cfme/tests/services/test_ansible_workflow_servicecatalogs.py
    • test_tower_workflow_item
    • test_retire_ansible_workflow

The local fixture catalog_item is used in the following files:

  • cfme/tests/services/test_config_provider_servicecatalogs.py
    • test_order_tower_catalog_item
    • test_retire_ansible_service

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@mshriver mshriver merged commit 3250c91 into ManageIQ:master Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants