Conversation
m-horky
commented
Jan 16, 2023
m-horky
commented
- Card ID: ENT-3759
cnsnyder
requested review from
a team and
jirihnidek
and removed request for
a team
m-horky
force-pushed
the
mhorky/ENT-3759_actions
branch
2 times, most recently
from
fd04b58 to
7358a48
Compare
ptoscano
left a comment
ptoscano
left a comment
There was a problem hiding this comment.
Looks like a good start!
I left some minor notes & questions here and there, nothing too problematic IMHO.
.github/workflows/pytest.yml
Outdated
| include: | ||
| - name: "CentOS Stream 9" | ||
| image: "quay.io/centos/centos:stream9" | ||
| pytest_args: '--deselect test/rhsmlib/facts/test_hwprobe.py::HardwareProbeTest::test_networkinfo --deselect test/rhsmlib_test/test_facts.py::TestFactsDBusObject::test_GetFacts' |
There was a problem hiding this comment.
hmm the second path (test/rhsmlib_test/test_facts.py) does not exist; i guess the second --deselect can be dropped altogether?
There was a problem hiding this comment.
This class and method is located in the following file: test/rhsmlib/dbus/test_facts.py. If there is good reason to not run following unit tests, then this reason should be shared in the comment of the code.
There was a problem hiding this comment.
Yeah, the directory name differs between main and 1.28, it slipped in by accident.
That D-Bus' test_GetFacts runs full fact collection, and it was triggering the same error as the test_networkinfo. I'll add it to the comment below so it is clear.
There was a problem hiding this comment.
though, if it worked in the first revision, then most likely the exclusion of test_GetFacts is not needed?
There was a problem hiding this comment.
It's difficult to say, sometimes it crashes, sometimes it does not. Whole segfault is not 100% reproducible, sometimes it politely errors out with OSError. I'd rather keep it here, to prevent false negatives.
test/rhsmlib/dbus/test_facts.py
Outdated
| name="get_virt_info", | ||
| ) | ||
| cls.patches["get_virt_info"] = get_virt_info_patch.start() | ||
| cls.patches["get_virt_info"].return_value = {} |
There was a problem hiding this comment.
this is set later in setUp(): it seems to me that the value there is static, so i think it'd be better to move that here instead; can you please add it as separate commit?
There was a problem hiding this comment.
I'd rather do it in some completely separate PR, but sure.
There was a problem hiding this comment.
ah sure, separate PR is what I had in mind -- waiting for it ;)
jirihnidek
left a comment
jirihnidek
left a comment
There was a problem hiding this comment.
I like it! 👍 I have few comments notes and suggestions too.
.github/workflows/pytest.yml
Outdated
| include: | ||
| - name: "CentOS Stream 9" | ||
| image: "quay.io/centos/centos:stream9" | ||
| pytest_args: '--deselect test/rhsmlib/facts/test_hwprobe.py::HardwareProbeTest::test_networkinfo --deselect test/rhsmlib_test/test_facts.py::TestFactsDBusObject::test_GetFacts' |
There was a problem hiding this comment.
This class and method is located in the following file: test/rhsmlib/dbus/test_facts.py. If there is good reason to not run following unit tests, then this reason should be shared in the comment of the code.
* Card ID: ENT-3759
m-horky
force-pushed
the
mhorky/ENT-3759_actions
branch
from
7358a48 to
48b5723
Compare
Function 'setUp()' was overwriting the dictionary set in 'setUpClass()' on every run for no good reason.
m-horky
force-pushed
the
mhorky/ENT-3759_actions
branch
from
48b5723 to
6187394
Compare
ptoscano
left a comment
ptoscano
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I'll let Jirka review this as well, as he had notes previously.
3 participants
