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

[NOTEST][WIP] Adding ReportParser class and miq blame command #9110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[NOTEST][WIP] Adding ReportParser class and miq blame command #9110

wants to merge 2 commits into from

Conversation

kedark3
Copy link
Contributor

@kedark3 kedark3 commented Jul 24, 2019 •
edited
Loading

Purpose or Intent

Adding miq blame command that will be able to take a Json file as input and return list of test cases per assignee and also provide failed tests report.

Usage: miq blame [OPTIONS] COMMAND [ARGS]...

  Functions for generating reports on Tests grouped by user

Options:
  --help  Show this message and exit.

Commands:
  test-report  Returns a Dictionary containing failed test cases per user


Usage: miq blame test-report [OPTIONS]

  Returns a Dictionary containing failed test cases per user

Options:
  --report-file TEXT              Path to the file with json data
  --email-config-yaml PATH        If you decide to use this option,copy yaml
                                  file similar to
                                  report_parser_email_conf.yaml.template with
                                  correct values and pass its path. No email
                                  would be sent if --user is not set.
  --user TEXT                     Set this if you want to filter the report
                                  for a specific user
  --filter-by [passed|failed|skipped]
                                  Defaults to 'failed'
  --help                          Show this message and exit.

@kedark3 kedark3 force-pushed the test_failure_parser branch from 327adfc to b2a8536 Compare July 24, 2019 15:34
@digitronik
Copy link
Contributor

If possible rename blame to codecov so I will use same command group for extension. I have one card which will provide codecov and cover commands like

  1. automate and manual % as per assignee
  2. automate and manual % as per FA
    etc...

Thanks for initialize this... I am facing problem with requirements but i will add basic implementation.

@kedark3
Copy link
Contributor Author

kedark3 commented Jul 24, 2019

@digitronik I have no issue renaming. @mshriver @jawatts what you guys think?

@kedark3 kedark3 force-pushed the test_failure_parser branch from b2a8536 to 4a0c8c2 Compare July 24, 2019 17:45
@kedark3 kedark3 changed the title [NOTEST][WIP]Adding ReportParser class and miq blame command [NOTEST][RFR]Adding ReportParser class and miq blame command Jul 24, 2019
@dajoRH dajoRH removed the WIP label Jul 24, 2019
@kedark3 kedark3 force-pushed the test_failure_parser branch 3 times, most recently from 2c797a3 to 3c7a863 Compare July 25, 2019 18:55
@dajoRH dajoRH changed the title [NOTEST][RFR]Adding ReportParser class and miq blame command [NOTEST][WIP]Adding ReportParser class and miq blame command Jul 25, 2019
@dajoRH dajoRH added needs-lint and removed lint-ok labels Jul 25, 2019
@kedark3 kedark3 force-pushed the test_failure_parser branch from 3c7a863 to 9c262f9 Compare July 25, 2019 19:03
@kedark3 kedark3 changed the title [NOTEST][WIP]Adding ReportParser class and miq blame command [NOTEST][RFR]Adding ReportParser class and miq blame command Jul 25, 2019
@dajoRH dajoRH added lint-ok and removed needs-lint labels Jul 25, 2019
john-dupuy
john-dupuy suggested changes Jul 29, 2019
View reviewed changes
cfme/scripting/report_parser.py Outdated Show resolved Hide resolved
cfme/scripting/report_parser.py Outdated Show resolved Hide resolved
@kedark3 kedark3 force-pushed the test_failure_parser branch from 9c262f9 to 6560fc4 Compare July 29, 2019 15:55
john-dupuy
john-dupuy reviewed Jul 29, 2019
View reviewed changes
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.

I cannot test this locally due to no json data, but I have some general comments and suggestions, please take a look

cfme/scripting/report_parser.py Show resolved Hide resolved
cfme/scripting/report_parser.py
pass


@main.command("test-report", help="Returns a Dictionary containing failed test cases per user")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on naming this something else? My concern is that this makes it sound like you're running a test. One thought is that the calling sequence could be miq blame <user> so that it makes it seem like you're literally blaming the person 😄 The all the other args could be optional. Not sure if it's worth the refactor though, so I'd like to hear your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@john-dupuy I'd like to get one more review on this before I refactor. Don't want to refactor only to go back to what it was. I like your idea too. :)

cfme/scripting/report_parser.py
default='')
@click.option("--filter-by", type=click.Choice(['passed', 'failed', 'skipped']),
help="Defaults to 'failed'", default='failed')
def test_results_per_assignee(report_file, email_config_yaml, user, filter_by):
Copy link
Contributor

Choose a reason for hiding this comment

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

When I run miq blame test-report, I get an Unhandled TypeError that doesn't really explain why my command failed, it'd be useful to have it print some required about the required options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making --report-file required would make it go away.

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 marked that arg as required so it should not give TypeError now.

cfme/scripting/report_parser.py Outdated
rp = ReportParser(report_file)
test_report_dict = rp.parse_report_per_user(filter_by, user)
# user is required before following routine would execute, else no email is sent
if test_report_dict and email_config_yaml and user:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anyway to generate the report for all users? Because if so I'd expect the default to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So by default user is '' so it fetches report for each user. And I am adding a default_to_addr so that if user is not defined, email would go to default_to_addr. Currently I derive email from username, but if we want to be explicit that would require more work. I understood the task as we will have our kerberos username in assignee.

cfme/scripting/report_parser.py Outdated Show resolved Hide resolved
cfme/scripting/report_parser.py Outdated Show resolved Hide resolved
cfme/scripting/report_parser.py Outdated
@click.option("--report-file", help="Path to the file with json data")
@click.option("--email-config-yaml", help="If you decide to use this option,copy yaml file similar"
" to report_parser_email_conf.yaml.template with correct values and pass its path."
" No email would be sent if --user is not set.",
Copy link
Contributor

Choose a reason for hiding this comment

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

No email will be sent if --user is not set -> shouldn't this be under the --user field?

cfme/scripting/report_parser.py Outdated

@main.command("test-report", help="Returns a Dictionary containing failed test cases per user")
@click.option("--report-file", help="Path to the file with json data")
@click.option("--email-config-yaml", help="If you decide to use this option,copy yaml file similar"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any default value for this option? I suggest that the help message simply read The path to your email configuration file, e.g. "report_parser_email_conf.yaml"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No default values really. But I updated the message.

cfme/scripting/report_parser.py Outdated


@main.command("test-report", help="Returns a Dictionary containing failed test cases per user")
@click.option("--report-file", help="Path to the file with json data")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any default value for this option?

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 think I can create something dummy, do we want that in the repo?

@kedark3 kedark3 force-pushed the test_failure_parser branch from 6560fc4 to 1a6d558 Compare July 29, 2019 19:41
@john-dupuy john-dupuy changed the title [NOTEST][RFR]Adding ReportParser class and miq blame command [NOTEST][WIPTEST] Adding ReportParser class and miq blame command Aug 15, 2019
@dajoRH dajoRH changed the title [NOTEST][WIPTEST] Adding ReportParser class and miq blame command [NOTEST][WIP] Adding ReportParser class and miq blame command Sep 17, 2019
@dajoRH
Copy link
Contributor

dajoRH commented Sep 17, 2019

Would you mind rebasing this Pull Request against latest master, please? :trollface:
CFME QE Bot

@dajoRH dajoRH added WIP and removed WIP-testing labels Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@john-dupuy john-dupuy john-dupuy requested changes

Assignees

@jawatts jawatts

Labels
lint-ok needs-rebase WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kedark3 @digitronik @dajoRH @john-dupuy @jawatts