add test inspector results into test library for nondestructive tests #19756
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| else: | ||
| self.log(ADD, f"key:{key} ", new_data[key]) | ||
|
|
||
| def _compare_string(self, old_data, new_data): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
| if old_data != new_data: | ||
| self.log(CHANGE, old_data, new_data) | ||
|
|
||
| def compare_multiline(self, old_data, new_data): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
test/common/testinsp/network.py
Outdated
| interface = item["ifname"] | ||
| adresses = list() | ||
| addrinfo_list = ["local", "prefixlen", "family", "label", "scope"] | ||
| for item in item["addr_info"]: |
Check failure
Code scanning / CodeQL
Nested loops with same variable reused after inner loop body Error test
test/common/testinsp/network.py
Outdated
| interface = item["ifname"] | ||
| adresses = list() | ||
| addrinfo_list = ["local", "prefixlen", "family", "label", "scope"] | ||
| for item in item["addr_info"]: |
Check notice
Code scanning / CodeQL
Nested loops with same variable Note test
| data = safe_dump(self.data) | ||
| elif self.store_type == PLAIN: | ||
| data = self.data | ||
| fd.write(data) |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable Error test
|
@jscotka that is a loooooot of code.. can we discuss the design of this first? However, tomorrow is the last workday for the remainder of the cockpit team this year, so perhaps start of January? |
There was a problem hiding this comment.
I had a quick look at the categories of information here. Thanks for this big effort, but I'm afraid this really isn't what's going to help us. The bits that surprise us are dynamic race conditions in event orders, udev, udisks, D-Bus signal orderings and so on. This kind of post-mortem data collection won't help us much there, I'm afraid, as there is almost never a doubt or difficulty about that.
Where useful, some tests gather/print specific and targeted extra information like here or here. But this is rare, mostly because in most cases of unexpected failures/races we can already see the visible end result of things going wrong in the journal and screenshot -- the hunt is about deducing (1) what's the root cause, and (2) creating a CLI reproducer for it for bug reporting (by far the hardest part).
As said, let's discuss this first what you had in mind, and for which failures this would have helped.
Thanks!
| @@ -0,0 +1,40 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
There is never any doubt about what the config files in /etc contain. This is fixed in the images and our testlibs, we don't need that.
| @@ -0,0 +1,52 @@ | |||
| from testinsp.base import TestInspector | |||
There was a problem hiding this comment.
This also has never been doubtful. We know which interfaces are set up at any point in the test, and screenshots tell us their actual state.
|
|
||
| class FirewallStatus(TestInspector): | ||
| store_type = YAML | ||
| _get_data_command = "firewall-cmd --list-all || true" |
There was a problem hiding this comment.
Same here, this usually isn't a surprise. If "cockpit" service is not enabled, it's blatantly clear from the test output, and also our test setup explicitly enables it. For other services, the TestFirewall failures show the current state.
|
|
||
| class ServiceInfo(TestInspector): | ||
| store_type = PLAIN | ||
| _get_data_command = "systemctl --type=service --state=running" |
There was a problem hiding this comment.
This is a lot of data, and just overwhelming. The vast majority of services have nothing to do with a particular test case, and just add a lot of noise to the failure logs. This makes more sense for specific services which are unexpectedly running/not running, such as udisks.service failing in the storage tests. But we already see all of this in the journal attachments, so it's redundant.
|
|
||
| class DiskInfo(TestInspector): | ||
| store_type = YAML | ||
| _get_data_command = "udisksctl dump" |
There was a problem hiding this comment.
This again falls into the "no surprise here" category. Our tests only ever work on scsi_debug or loop devices, their setup is in --verbose --trace and also in the journal. This is also redundant.
There was a problem hiding this comment.
Note: There might very well be specific tests where this is unclear, but it should then be restricted to a particular test or at most storagelib.py. But this should be triggered by a failure where it would have helped. Every piece of information which we attach to a failure has a cost -- humans need to trawl through it and find the things that actually matter. Even then we usually only add it temporarily in a draft PR for examination (if it doesn't reproduce locally), and in most cases take it out again.
martinpitt
added
the
no-test
Test of test inspector