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

[RFR] Updated bz coverage utility to not update bugs in coverage= marker #10130

Merged
merged 8 commits into from
May 21, 2020

Conversation

prichard77
Copy link
Contributor

@prichard77 prichard77 commented May 19, 2020

Purpose or Intent

  • Updating test utility miq bz coverage so that it no longer updates the qe_test_coverage flag in bugzilla for bugs in coverage= markers within tests. Also replacing log messages with exceptions when bugzilla credentials are missing.

PRT Run

Suggestions on what to use here?

…= and replaced log msg with exception when no bz creds
@prichard77
Copy link
Contributor Author

These updates were made quickly with getting the utility working so we could update bugs being the priority. I wanted to get it out for review comments. I am trying to determine where we want to catch the Exceptions thrown if we have no bz creds, and what specifically to do when caught. Thinking we want to write something to store.terminalreporter and end the test, but from where?

And I want to make sure, when I removed the coverage= code , I didn't create an issue with something else using the code I updated. Seems unlikely, but I have no idea.

Comment on lines 29 to 30
if "automates" not in item._metadata and "coverage" not in item._metadata:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

@prichard77 rather than removing the handling of the coverage/manual cases altogether, I think we should make it an option (--include-manual) that defaults to False.

Something like

group.addoption(
        '--include-manual',
        action='store_true',
        default=False,
        dest='include_manual',
        help='Include manual coverage in the report'
    )

Copy link
Contributor

@john-dupuy john-dupuy May 19, 2020

Choose a reason for hiding this comment

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

Then the miq bz command can also have an --include-manual flag that can trickle into the pytest.main call in get_report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a great idea. working on it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at where this is called from, there already is a --include-manual option being used in get_report(), line 55. I am assuming we want to default to not using this option and add the ability to use it as needed from the CL.

Copy link
Contributor

Choose a reason for hiding this comment

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

OH that's right, we should just use that! So we can just add an --include-manual flag in miq bz coverage and then include it in the pytest.main call based on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

That way no changes are required to cfme.fixtures.bzs.py

@prichard77
Copy link
Contributor Author

I removed my initial changes. Now only scripting.py has been updated (still have to work on checking for creds). I added an option to coverage. I have done a little testing on this, but it's tough to test thoroughly as we have already updated all of the Bugs to reflect the correct/current state. There is no work to be done by the utility now.

cfme/scripting/bz.py Outdated Show resolved Hide resolved
@@ -224,6 +225,15 @@ def list(directory, bz_status):

@main.command(help="Set QE test coverage flag based on automates/coverage metadata")
@click.argument("directory", default="cfme/tests")
@click.option(
"-m",
"--inc_man",
Copy link
Contributor

Choose a reason for hiding this comment

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

@prichard77 for consistency with the pytest arg, I suggest we just call this --include-manual

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

@prichard77
Copy link
Contributor Author

I was going to make sure I added --inc_manual to all of the functions where it was applicable, but it looks like all the others are fine like they are. All others are specifically looking for 'coverage' so turning it off would make no sense. Please let me know if I missed one for which the option should apply.

@prichard77 prichard77 changed the title [WIP] Updated bz coverage utility to not update bugs in coverage= marker [RFR] Updated bz coverage utility to not update bugs in coverage= marker May 20, 2020
cfme/scripting/bz.py Outdated Show resolved Hide resolved
@dajoRH dajoRH removed the WIP label May 20, 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.

LGTM, just one suggestion, but I'd consider it optional

cfme/scripting/bz.py Outdated Show resolved Hide resolved
cfme/scripting/bz.py Outdated Show resolved Hide resolved
cfme/scripting/bz.py Outdated Show resolved Hide resolved
cfme/scripting/bz.py Outdated Show resolved Hide resolved
Updating arg to include_manual

Co-authored-by: john-dupuy <44065123+john-dupuy@users.noreply.github.com>
cfme/scripting/bz.py Outdated Show resolved Hide resolved
missed one of the include_manual changes
@mshriver mshriver merged commit 2b7a842 into ManageIQ:master May 21, 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.

None yet

4 participants