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

[RFR] Automate test: test_reports_in_global_region#10246

Merged
mshriver merged 3 commits intoManageIQ:masterfrom
valaparthvi:report_multi_region
Jul 17, 2020
Merged

[RFR] Automate test: test_reports_in_global_region#10246
mshriver merged 3 commits intoManageIQ:masterfrom
valaparthvi:report_multi_region

Conversation

@valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Jul 9, 2020

Purpose or Intent

  • Adding tests
    1. test_reports_in_global_region.
  • Enhancement
    1. Add rest_api_entity property to Report.
    2. cfme/tests/intelligence/reports/test_reports.py::report fixture to take custom appliance
    3. Add fixtures -
      1. cfme/fixtures/appliance.py::temp_appliance_preconfig_modscope_rhevm
      2. cfme/fixtures/cli.py::replicated_appliances_modscope

PRT Run

{{ pytest: cfme/tests/intelligence/reports/test_reports.py::test_reports_in_global_region -svvv --long-running}}

@valaparthvi valaparthvi added the test-automation To be applied on PR's which are automating existing manual cases label Jul 9, 2020
@valaparthvi valaparthvi force-pushed the report_multi_region branch 2 times, most recently from f73fab9 to b9fbd83 Compare July 9, 2020 15:34
@valaparthvi valaparthvi force-pushed the report_multi_region branch from b9fbd83 to c7ed3a5 Compare July 9, 2020 15:44
@valaparthvi valaparthvi changed the title [WIPTEST] Automate test: test_reports_in_global_region [RFR] Automate test: test_reports_in_global_region Jul 10, 2020
@valaparthvi
Copy link
Contributor Author

@tpapaioa can you review this?

remote_appliance, global_appliance = replicated_appliances_modscope
logger.info("Starting appliance replication configuration.")
second_remote_appliance.configure(region=89, key_address=remote_appliance.hostname)
second_remote_appliance.set_pglogical_replication(replication_type=":remote")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting up some of the appliances in replicated_appliances_modscope and some of them here, I think it would make more sense to have all of the db configuration and replication configuration in one fixture. The fixtures in cfme/fixtures/multi_region.py could be used here; multi_region_cluster can pass an indirect parameter to temp_appliances_unconfig_modscope_rhevm to specify the total number of appliances, and setup_multi_region_cluster takes care of configuring all of them. There is also a setup_remote_provider fixture that could be copied or modified to set up providers on multiple remote appliances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was awesome! Thanks for the suggestions @tpapaioa, let me know how it looks now.

@mshriver mshriver changed the title [RFR] Automate test: test_reports_in_global_region [WIPTEST] Automate test: test_reports_in_global_region Jul 10, 2020
@valaparthvi valaparthvi force-pushed the report_multi_region branch from 4407bc4 to cc45192 Compare July 13, 2020 12:03
@valaparthvi valaparthvi changed the title [WIPTEST] Automate test: test_reports_in_global_region [RFR] Automate test: test_reports_in_global_region Jul 13, 2020
@valaparthvi valaparthvi force-pushed the report_multi_region branch from cc45192 to 5a0980f Compare July 13, 2020 14:51
@tpapaioa
Copy link
Contributor

Looks good. Just two small additional comments:

  • setup_providers_on_multi_region_cluster returns multi_region_cluster as the third tuple entry, even though the fixture didn't create or modify it. This fixture should just return the two providers, and if the test or any other fixtures / methods need multi_region_cluster, they can accept it as a separate parameter.

  • temp_appliances_unconfig_modscope_rhevm doesn't need to be in the parameter list for test_reports_in_global_region, since it's not used in the test method and is already called during the setup of multi_region_cluster. The indirect parametrize call will still work as is.

@valaparthvi valaparthvi force-pushed the report_multi_region branch from 5a0980f to 9822db6 Compare July 15, 2020 06:15
@valaparthvi valaparthvi force-pushed the report_multi_region branch from 9822db6 to 14df6a1 Compare July 15, 2020 07:20
@dajoRH
Copy link
Contributor

dajoRH commented Jul 15, 2020

I detected some fixture changes in commit 14df6a1

The local fixture setup_providers_on_multi_region_cluster is used in the following files:

  • cfme/tests/intelligence/reports/test_reports.py

The local fixture saved_report is used in the following files:

  • cfme/tests/intelligence/reports/test_reports.py

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

@mshriver mshriver self-assigned this Jul 15, 2020
@valaparthvi
Copy link
Contributor Author

@tpapaioa updated. I can't believe I missed out on those things you suggested while writing the test 🤦

@tpapaioa
Copy link
Contributor

Looks good to me.

@mshriver mshriver merged commit 3d3fd6d into ManageIQ:master Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement test-automation To be applied on PR's which are automating existing manual cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants