Skip to content

feat: junit report directory#413

Merged
Pog3k merged 39 commits into
eclipse-kiso-testing:masterfrom
IliyanKordev:feature/junit_report_directory
Dec 1, 2023
Merged

feat: junit report directory#413
Pog3k merged 39 commits into
eclipse-kiso-testing:masterfrom
IliyanKordev:feature/junit_report_directory

Conversation

@IliyanKordev
Copy link
Copy Markdown
Contributor

No description provided.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (61e219e) 98.11% compared to head (1c3342e) 98.11%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files          80       80           
  Lines        6471     6487   +16     
=======================================
+ Hits         6349     6365   +16     
  Misses        122      122           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

@sebastianpfischer sebastianpfischer left a comment

Choose a reason for hiding this comment

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

The coverage report is not generated correctly anymore

Comment thread src/pykiso/cli.py Outdated
# filter flags
flags = [
o
for o in self.params
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please change the naming from o to param. Same for some of the other loops below, it's best to give descriptive names instead of single character names

Copy link
Copy Markdown
Contributor Author

@IliyanKordev IliyanKordev Nov 15, 2023

Choose a reason for hiding this comment

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

Ok, i will fix names

step_report: Optional[Path] = None,
pattern_inject: Optional[str] = None,
failfast: bool = False,
junit_path: str = "reports",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

update docstring for junit_path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, i will fix it

reports_path = project_folder / "reports"
junit_report_path = reports_path / junit_report_name
reports_path.mkdir(exist_ok=True)
report_path = Path(junit_path)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Somehow you broke the old behaviour.

If I run
pykiso -c .\examples\dummy.yaml --junit
the report is now created at the location where pykiso has been started. Before those files where in the
"reports" folder. I would expect that same behaviour if user dont specify any path...

Something like this
pykiso -c .\examples\dummy.yaml --junit customname.xml doesnt work at all.

pykiso --help also doesnt show the optional flag.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, i fixed it

reports_path = project_folder / "reports"
junit_report_path = reports_path / junit_report_name
reports_path.mkdir(exist_ok=True)
report_path = junit_path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need the renaming ?
Just proceed with junit_path instead of report_path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, i will rename it

Comment thread testfolder1/f.xml Outdated
@@ -0,0 +1,107 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this folder and all its contents

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, i will remove it

@sebastianpfischer
Copy link
Copy Markdown
Contributor

@Pog3k, if you merge it before I do, please squash the PR before

@sebastianpfischer
Copy link
Copy Markdown
Contributor

Sorry, I start reading the PR, I do not understand what you try to do. Could you please explain it to me?

junit_report_path = reports_path / junit_report_name
reports_path.mkdir(exist_ok=True)
report_path = junit_path
full_report_path = Path.cwd() / report_path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That would not work if I drop the location, I will get a absolute path instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what do you mean ?
I mean I looks weird but if report_path is an abspath Path.cwd() gets ignored...
tested on windows and linux

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really, cool, still please comment on top, because yes, it looks really weird ^^

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code documentation, like mention above, still missing

Comment thread src/pykiso/cli.py Outdated
Comment thread src/pykiso/cli.py Outdated
and flag.is_flag
and not isinstance(flag.flag_value, bool)
]
junit_prefixes = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this somehow be made generic?
It feels weird to have a class named CommandWithOptionalFlagValues (very generic name) that only handles one specific flag name --junit

Comment thread src/pykiso/cli.py Outdated
IliyanKordev and others added 5 commits November 21, 2023 09:45
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Comment thread src/pykiso/cli.py Outdated
Comment thread src/pykiso/cli.py Outdated
Comment thread src/pykiso/cli.py
Comment on lines +93 to +118
class CommandWithOptionalFlagValues(click.Command):
def parse_args(self, ctx, args):
"""Translate any flag `--junit=value` as flag `--junit` with changed flag_value=value"""
# filter flags
flags = [
flag
for flag in self.params
if isinstance(flag, click.Option)
and flag.is_flag
and not isinstance(flag.flag_value, bool)
]
junit_prefixes = {
flag_str: flag
for flag in flags
for flag_str in flag.opts
if flag_str.startswith("--junit")
}

for index, arg in enumerate(args):
arg = arg.split("=")
if len(arg) < 2:
continue
junit_prefixes[arg[0]].flag_value = arg[1]
args[index] = arg[0]
result_args = super(CommandWithOptionalFlagValues, self).parse_args(ctx, args)
return result_args
Copy link
Copy Markdown
Contributor

@sebclrsn sebclrsn Nov 24, 2023

Choose a reason for hiding this comment

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

To make this generic:

Suggested change
class CommandWithOptionalFlagValues(click.Command):
def parse_args(self, ctx, args):
"""Translate any flag `--junit=value` as flag `--junit` with changed flag_value=value"""
# filter flags
flags = [
flag
for flag in self.params
if isinstance(flag, click.Option)
and flag.is_flag
and not isinstance(flag.flag_value, bool)
]
junit_prefixes = {
flag_str: flag
for flag in flags
for flag_str in flag.opts
if flag_str.startswith("--junit")
}
for index, arg in enumerate(args):
arg = arg.split("=")
if len(arg) < 2:
continue
junit_prefixes[arg[0]].flag_value = arg[1]
args[index] = arg[0]
result_args = super(CommandWithOptionalFlagValues, self).parse_args(ctx, args)
return result_args
class CommandWithOptionalFlagValues(click.Command):
def parse_args(self, ctx, args):
"""Translate any flag `--junit=value` as flag `--junit` with changed flag_value=value"""
# filter flags
flags = [
flag
for flag in self.params
if isinstance(flag, click.Option)
and flag.is_flag
and not isinstance(flag.flag_value, bool)
]
for arg_index, arg in enumerate(args):
arg = arg.split("=")
if len(arg) != 2:
continue
arg_name, arg_value = arg
for flag in flags:
if arg_name in flag.opts:
flag.flag_value = arg_value
args[arg_index] = arg_name
break
result_args = super(CommandWithOptionalFlagValues, self).parse_args(ctx, args)
return result_args

IliyanKordev and others added 5 commits November 27, 2023 10:27
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@sebclrsn sebclrsn left a comment

Choose a reason for hiding this comment

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

Just add an entry in the what's new that explains these changes and we're good to go, e.g.:

The ``--junit`` option can now be provided with a value, indicating the folder or file where the report should be written, e.g. ``pykiso -c my_config.yaml --junit=./reports/my_config.xml``.
This is now the preferred way of setting the JUnit reports path, and providing only ``--junit`` will progressively be deprecated in the future.

Comment thread src/pykiso/cli.py
return paths


class CommandWithOptionalFlagValues(click.Command):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Documentation, types missing.
Also documentation of the code too.

junit_report_path = reports_path / junit_report_name
reports_path.mkdir(exist_ok=True)
report_path = junit_path
full_report_path = Path.cwd() / report_path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code documentation, like mention above, still missing

@IliyanKordev
Copy link
Copy Markdown
Contributor Author

Sorry, I start reading the PR, I do not understand what you try to do. Could you please explain it to me?

I think the confusing part is that --junit was just a flag in the past. To dont introduce a breaking change or an addtional flag the call now looks like this --junit=my.xml So Both calls are now valid pykiso -c .\examples\dummy.yaml --junit=my.xml pykiso -c .\examples\dummy.yaml --junit
Or what is still unclear ?

This is actually cool but why not simply a -o like all the other tools do?

Because, it is easier and more clear for the teams, without adding new arguments of pykiso

Copy link
Copy Markdown
Contributor

@sebclrsn sebclrsn left a comment

Choose a reason for hiding this comment

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

Just apply the code comments for understanding and we're good to go

Comment thread src/pykiso/cli.py Outdated
Comment thread src/pykiso/cli.py
Comment thread src/pykiso/cli.py
Comment thread src/pykiso/cli.py
IliyanKordev and others added 4 commits November 30, 2023 13:53
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
Co-authored-by: Sebastian Clerson <58192998+sebclrsn@users.noreply.github.com>
@Pog3k Pog3k merged commit ef87422 into eclipse-kiso-testing:master Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants