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

[1LP][RFR] Add comparison of OS data after revert to test_verify_revert_snapshot #10176

Merged
merged 9 commits into from
Jun 23, 2020

Conversation

prichard77
Copy link
Contributor

@prichard77 prichard77 commented Jun 8, 2020

Purpose or Intent

  • Updating tests to add automated coverage for test_snapshot_image_copies_system_info manual test.

PRT Run

{{ pytest: --long-running cfme/tests/infrastructure/test_snapshot.py::test_verify_revert_snapshot }}

@prichard77
Copy link
Contributor Author

How do we want to add this functionality? Do we want to:

  • Follow current implementation?
  • Add verify functionality to verify_revert_snapshot and turn it on in test_verify_revert_snapshot (via kwarg defaulting to False)?
  • Add verify functionality to verify_revert_snapshot and turn it on in a new test case (via kwarg defaulting to False)?
  • Write entirely new test case that does not use verify_revert_snapshot().

@prichard77 prichard77 changed the title [WIP]Add comparison of OS data after revert to test_verify_revert_snapshot [WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 9, 2020
@prichard77 prichard77 changed the title [WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot [RFR]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 10, 2020
@john-dupuy
Copy link
Contributor

@prichard77 I think the current implementation is good. If we want to add this check to other test cases then maybe option (2) or (3) would be appropriate. But if this is the only place where we want to check the OS info, then option (1) is the way to go imo.

@john-dupuy john-dupuy changed the title [RFR]Add comparison of OS data after revert to test_verify_revert_snapshot [WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 16, 2020
@prichard77 prichard77 changed the title [WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot [RFR]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 17, 2020
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

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

Two questions about removing the comments, otherwise LGTM 👍

cfme/infrastructure/virtual_machines.py Outdated Show resolved Hide resolved
cfme/tests/infrastructure/test_snapshot.py Outdated Show resolved Hide resolved
@john-dupuy john-dupuy changed the title [RFR]Add comparison of OS data after revert to test_verify_revert_snapshot [1LP][RFR]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 18, 2020

@property
def is_displayed(self):
expected_title = '"OS Info" for Virtual Machine "{}"'.format(self.context['object'].name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use f string 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.

I try to use f string and was wondering why it wasn't used here, until I tried to implement it. There are too many quotes. I started down the escape character route, but that makes things real ugly. If you have an easier solution, let me know. The 'object' quotes would need to be inside on double quotes and the entire string is inside of single. We need 3 types of quotes.

Copy link
Contributor

@john-dupuy john-dupuy Jun 19, 2020

Choose a reason for hiding this comment

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

@prichard77 I think you could split it into steps

        name = self.context['object'].name
        expected_title = f'"OS Info" for Virtual Machine "{name}"'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but is this really better than the old format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and updated to f' .

@jawatts
Copy link
Contributor

jawatts commented Jun 19, 2020

PRT did not run any tests, @prichard77 can you look into why

@prichard77
Copy link
Contributor Author

Looking into why nothing ran in PRT. Tests were deselected. I am looking into why now.
collected 2 items / 2 deselected

@prichard77
Copy link
Contributor Author

I really with the PRT messages/logs were clearer on what exactly is happening. There is a blocker for rhv providers but vsphere is not blocked. However, when I try to run vsphere locally I get an error trying to setup the provider.

@prichard77
Copy link
Contributor Author

I believe the PRT issue was that I was missing --long-running. I also made another update to the markers to only use vsphere providers. I originally had a blocker marker for just rhv providers, but the BZ was closed WONTFIX . The bug only occurs for rhv.

@jawatts jawatts changed the title [1LP][RFR]Add comparison of OS data after revert to test_verify_revert_snapshot [1LP][WIP]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 19, 2020
@prichard77 prichard77 changed the title [1LP][WIP]Add comparison of OS data after revert to test_verify_revert_snapshot [1LP][WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot Jun 20, 2020
@prichard77 prichard77 changed the title [1LP][WIPTEST]Add comparison of OS data after revert to test_verify_revert_snapshot [1LP][RFR] Add comparison of OS data after revert to test_verify_revert_snapshot Jun 20, 2020
@prichard77 prichard77 requested a review from jawatts June 22, 2020 18:31
blockers=[BZ(1805803, unblock=lambda provider: not provider.one_of(RHEVMProvider),
ignore_bugs={1745065})], automates=[1805803])
@pytest.mark.rhv1
@pytest.mark.provider([VMwareProvider])
Copy link
Member

Choose a reason for hiding this comment

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

I think if we're going to remove RHV from the parametrization here, we should include comments concerning the BZ's, the fact that they aren't getting fixed, etc.

The other option here is to use pytest.mark.uncollectif, evaluating the provider type and BZ blocking status to uncollect the test case instead of skipping it.

This will make it so the test case isn't collected and included in reports, when it will not get fixed, instead of just being skipped at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uncollectif added.

@mshriver mshriver merged commit aafc995 into ManageIQ:master Jun 23, 2020
dgaikwad pushed a commit to dgaikwad/integration_tests that referenced this pull request Jun 24, 2020
…rt_snapshot (ManageIQ#10176)

* Add comparison of OS data after revert to test_verify_revert_snapshot

* Added verify_revert_snapshot() back into test. Had been removed for testing

* Add verification of OS data in OS view

* Create navigation to InfraVmOsView and add to test_verify_revert_snapshot

* Remove and update comments

* Remove more comments

* Update format of expected_title

* Remove blocker marker and add one to use only vsphere providers

* Update marker for test_verify_revert_snapshot
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.

4 participants