Add full_testcase_name parameter#4333
Add full_testcase_name parameter#4333nickzhq wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new option --vt-keep-at-variants-in-shortnames to preserve @-prefixed variants in short names. The changes are implemented across several files to add the command-line option, register it in the configuration, and pass it down to the cartesian_config.Parser where the logic is applied. The implementation appears correct and well-integrated. I have one suggestion to improve the code quality by addressing a mutable default argument in the Parser's __init__ method.
| def __init__(self, filename=None, defaults=False, expand_defaults=[], debug=False, keep_at_variants_in_shortnames=False): | ||
| self.node = Node() | ||
| self.debug = debug | ||
| self.defaults = defaults | ||
| self.expand_defaults = [LIdentifier(x) for x in expand_defaults] |
There was a problem hiding this comment.
The __init__ method uses a mutable default argument expand_defaults=[]. This is a common pitfall in Python that can lead to unexpected behavior, as the same list object is shared across all calls that don't explicitly provide a value for expand_defaults.
While I see that the pylint warning W0102 for this is disabled, it's a good practice to avoid mutable default arguments to prevent potential bugs.
A safer approach is to use None as the default value and initialize an empty list inside the method if None is received.
| def __init__(self, filename=None, defaults=False, expand_defaults=[], debug=False, keep_at_variants_in_shortnames=False): | |
| self.node = Node() | |
| self.debug = debug | |
| self.defaults = defaults | |
| self.expand_defaults = [LIdentifier(x) for x in expand_defaults] | |
| def __init__(self, filename=None, defaults=False, expand_defaults=None, debug=False, keep_at_variants_in_shortnames=False): | |
| if expand_defaults is None: | |
| expand_defaults = [] | |
| self.node = Node() | |
| self.debug = debug | |
| self.defaults = defaults | |
| self.expand_defaults = [LIdentifier(x) for x in expand_defaults] |
nickzhq
force-pushed
the
keep_at_default
branch
5 times, most recently
from
b2692c9 to
8d3ce9f
Compare
| help_msg=help_msg, | ||
| ) | ||
|
|
||
| help_msg = ( |
There was a problem hiding this comment.
Look those code changes are not necessary since you already implemented them in add_basic_vt_options of avocado_vt/plugins/vt.py
|
|
| add_option( | ||
| parser, | ||
| dest="vt.keep_at_variants_in_shortnames", | ||
| arg="--vt-keep-at-variants-in-shortnames", |
There was a problem hiding this comment.
Hi @nickzhq
As we discussed, actually, we do not need to add a new option to implement this functionality; instead, we could create a new item(e.g: fullname) to display the complete name for the return dictionary from the process of the cartesian_parser.get_dicts().
Thanks.
nickzhq
force-pushed
the
keep_at_default
branch
from
8d3ce9f to
c029e0d
Compare
nickzhq
changed the title
nickzhq
force-pushed
the
keep_at_default
branch
3 times, most recently
from
4f8c4ff to
531f351
Compare
|
The new parameter is params["full_testcase_name"], the format is as follows: |
nickzhq
force-pushed
the
keep_at_default
branch
from
531f351 to
420b4ae
Compare
|
Hello @Yingshun @YongxueHong @chunfuwen , please take a review, thanks! |
chunfuwen
left a comment
chunfuwen
left a comment
There was a problem hiding this comment.
`
import re
test_case1 = 'type_specific.io-github-autotest-qemu.guest_iommu_test.eim_off.default'
test_case2 = 'io-github-autotest-qemu.boot'
t1 = re.sub(r"^(type_specific.)?(io-[^.]+.)", "", test_case1)
t1
'guest_iommu_test.eim_off.default'
t1 = re.sub(r"^(type_specific.)?(io-[^.]+.)", "", test_case2)
t1
'boot'
`
After validation, the code looks goods.
|
@YongxueHong @PaulYuuu Please prioritize the review of this PR so we can proceed with the remaining KAR related tasks. We are currently blocked, awaiting its approval and merge. Thx! |
YvanY0
left a comment
YvanY0
left a comment
There was a problem hiding this comment.
Have a short talk with @YongxueHong, we all think the fullname should not cut from the short name, the pattern may take mistake.
The solution is find out where _short_name_map_file be generated, and add the new fullname from that code block.
@gemini-code-assist, please help to show where generates _short_name_map_file, and show us the logic, and the fullname should be added in the near code block, not using re module.
Hi @Yingshun |
Hello @YongxueHong @PaulYuuu , I got your point. The |
diff --git a/virttest/cartesian_config.py b/virttest/cartesian_config.py
index 95a5ee56..0176ae56 100755
--- a/virttest/cartesian_config.py
+++ b/virttest/cartesian_config.py
@@ -140,7 +140,7 @@ import re
import sys
_reserved_keys = set(
- ("name", "shortname", "dep", "_short_name_map_file", "_name_map_file")
+ ("name", "shortname", "dep", "_short_name_map_file", "_name_map_file", "testcase_id")
)
options = None
num_failed_cases = 5
@@ -521,6 +521,7 @@ class Node(object):
"children",
"labels",
"append_to_shortname",
+ "append_to_testcase_id",
"failed_cases",
"default",
"q_dict",
@@ -535,6 +536,7 @@ class Node(object):
self.children = []
self.labels = set()
self.append_to_shortname = False
+ self.append_to_testcase_id = False
self.failed_cases = collections.deque()
self.default = False
@@ -1789,6 +1791,11 @@ class Parser(object):
node3.append_to_shortname = not is_default
+ # Mark variants from subtests.cfg to be included in testcase_id
+ node3.append_to_testcase_id = (
+ lexer.filename != "<string>" and os.path.basename(lexer.filename) == "subtests.cfg"
+ )
+
op = LUpdateFileMap()
op.set_operands(
lexer.filename, ".".join(str(x) for x in node3.name)
@@ -1999,12 +2006,13 @@ class Parser(object):
self._debug("%s %s: %s" % (lexer.filename, lexer.linenum, lexer.line))
raise
- def get_dicts(self, node=None, ctx=[], content=[], shortname=[], dep=[]):
+ def get_dicts(self, node=None, ctx=[], content=[], shortname=[], dep=[], testcase_id=[]):
"""
Process 'join' entry, unpack join filter for node.
:param ctx: node labels/names
:param content: previous content in plain
+ :param testcase_id: accumulated testcase_id parts from subtests.cfg variants
:returns: dictionary
1) join filter_1 filter_2 ....
@@ -2050,7 +2058,7 @@ class Parser(object):
if not joins:
# Return generator
- for d in self.get_dicts_plain(node, ctx, content, shortname, dep):
+ for d in self.get_dicts_plain(node, ctx, content, shortname, dep, testcase_id):
yield _drop_suffixes(d) if parent else d
else:
# Rewrite all separate joins in one node as many `only'
@@ -2063,7 +2071,7 @@ class Parser(object):
old_content = node.content[:]
node.content = new_content
- for d in self.multiply_join(onlys, node, ctx, content, shortname, dep):
+ for d in self.multiply_join(onlys, node, ctx, content, shortname, dep, testcase_id):
yield _drop_suffixes(d) if parent else d
node.content = old_content[:]
@@ -2079,7 +2087,7 @@ class Parser(object):
name = p1 + "." + p2
return name
- def multiply_join(self, onlys, node=None, ctx=[], content=[], shortname=[], dep=[]):
+ def multiply_join(self, onlys, node=None, ctx=[], content=[], shortname=[], dep=[], testcase_id=[]):
"""
Multiply all joins. Return dictionaries one by one
Each `join' is the same as `only' filter
@@ -2093,14 +2101,14 @@ class Parser(object):
node.content += only
if not remains:
- for d in self.get_dicts_plain(node, ctx, content, shortname, dep):
+ for d in self.get_dicts_plain(node, ctx, content, shortname, dep, testcase_id):
yield d
else:
- for d1 in self.get_dicts_plain(node, ctx, content, shortname, dep):
+ for d1 in self.get_dicts_plain(node, ctx, content, shortname, dep, testcase_id):
# Current frame multiply by all variants from bottom
node.content = content_orig
for d2 in self.multiply_join(
- remains, node, ctx, content, shortname, dep
+ remains, node, ctx, content, shortname, dep, testcase_id
):
d = d1.copy()
@@ -2109,11 +2117,12 @@ class Parser(object):
d["shortname"] = self.mk_name(d1["shortname"], d2["shortname"])
yield d
- def get_dicts_plain(self, node=None, ctx=[], content=[], shortname=[], dep=[]):
+ def get_dicts_plain(self, node=None, ctx=[], content=[], shortname=[], dep=[], testcase_id=[]):
"""
Generate dictionaries from the code parsed so far. This should
be called after parsing something.
+ :param testcase_id: accumulated testcase_id parts from subtests.cfg variants
:return: A dict generator.
"""
@@ -2217,6 +2226,11 @@ class Parser(object):
ctx = ctx + node.name
ctx_set = set(ctx)
labels = node.labels
+
+ # Update testcase_id (only for variants from subtests.cfg)
+ if node.append_to_testcase_id:
+ testcase_id = node.name if node.var_name == "subtest" else testcase_id + node.name
+
# Get the current name
name = ".".join([str(label) for label in ctx])
@@ -2257,23 +2271,25 @@ class Parser(object):
count = 0
if self.defaults and node.var_name not in self.expand_defaults:
for n in node.children:
- for d in self.get_dicts(n, ctx, new_content, shortname, dep):
+ for d in self.get_dicts(n, ctx, new_content, shortname, dep, testcase_id):
count += 1
yield d
if n.default and count:
break
else:
for n in node.children:
- for d in self.get_dicts(n, ctx, new_content, shortname, dep):
+ for d in self.get_dicts(n, ctx, new_content, shortname, dep, testcase_id):
count += 1
yield d
# Reached leaf?
if not node.children:
self._debug(" reached leaf, returning it")
+
d = {
"name": name,
"dep": dep,
"shortname": ".".join([str(sn.name) for sn in shortname]),
+ "testcase_id": ".".join([str(t.name) for t in testcase_id]),
}
for _, _, op in new_content:
op.apply_to_dict(d)This version will introduce the And let me explain why we need to include provider prefix here. The problem is name collisions across test providers(tp-qemu/tp-libvirt). e.g., the Adding provider prefix willnot break anything, in tp-qemu test loop we always drop |
nickzhq
force-pushed
the
keep_at_default
branch
from
420b4ae to
f3cfbe9
Compare
So... close this patch and use |
nickzhq
force-pushed
the
keep_at_default
branch
2 times, most recently
from
3bc76e5 to
9228b8f
Compare
No, |
nickzhq
force-pushed
the
keep_at_default
branch
2 times, most recently
from
b2d0667 to
3c24500
Compare
I got confused by the whole comments on this patch. Or, you guys prefer |
nickzhq
force-pushed
the
keep_at_default
branch
from
3c24500 to
8df2461
Compare
nickzhq
force-pushed
the
keep_at_default
branch
3 times, most recently
from
c906fb2 to
cf3d4c9
Compare
|
Hi @YongxueHong @PaulYuuu, after a deep debugging, I found the DEBUG: _short_name_map_file = {'machines.cfg': 'q35', 'subtests.cfg': 'io-github-autotest-qemu.boot', 'guest-os.cfg': 'Guest.Linux.RHEL.10.1.x86_64', 'guest-mode.cfg': 'package', 'guest-hw.cfg': '[bridge.ovmf.no](http://bridge.ovmf.no/)_[virtio_rng.filesystem.no](http://virtio_rng.filesystem.no/)_[9p_export.smallpages.no](http://9p_export.smallpages.no/)_pci_assignable.qcow2.virtio_scsi.up.virtio_net', 'guest-hw-internal.cfg': 'vnc', 'host-os-internal.cfg': 'HostCpuVendor.intel.Host_RHEL.m10.u2.Hx86_64', 'tests-example.cfg': 'run_test'} Note: |
|
@Yingshun please use this patch for test. Only Thanks! |
Copies subtests.cfg value to subtest_id in the existing _short_name_map_file dictionary at dict generation time, preserving all subtest labels including type_specific and io-* provider prefixes. Usage: params["subtest_id"] Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Houqi (Nick) Zuo <hzuo@redhat.com>
nickzhq
force-pushed
the
keep_at_default
branch
from
cf3d4c9 to
74c4aa0
Compare
Hi @nickzhq |
|
Since the requirements had been changed last week, this patch is NOT needed. |
5 participants
ID: KVMAUTOMA-5109