cover pkg/ipmi 2 try

[ChriMarMe]

• Abstract syscall implementation behind interface
• Hide syscalls behind interface to improve testability
• Add mock tests for ipmi
• Add vm tests for ipmi
  Signed-off-by: Christopher Meis christopher.meis@9elements.com

[ChriMarMe]

#2182 (comment)

[codecov]

Codecov Report

Merging #2252 (a04ace4) into main (95ec011) will increase coverage by 0.12%.
The diff coverage is 70.12%.

@@            Coverage Diff             @@
##             main    #2252      +/-   ##
==========================================
+ Coverage   67.02%   67.14%   +0.12%     
==========================================
  Files         381      383       +2     
  Lines       39582    39615      +33     
==========================================
+ Hits        26529    26599      +70     
+ Misses      11970    11916      -54     
- Partials     1083     1100      +17     

Impacted Files	Coverage Δ
pkg/ipmi/ipmi.go	40.10% <23.07%> (+40.10%)	⬆️
pkg/ipmi/dev.go	92.85% <92.85%> (ø)
pkg/ipmi/handler.go	100.00% <100.00%> (ø)
pkg/ipmi/ipmi_linux.go	66.66% <100.00%> (+66.66%)	⬆️
pkg/acpi/sdt.go	0.00% <0.00%> (-76.67%)	⬇️
pkg/acpi/bios.go	0.00% <0.00%> (-67.57%)	⬇️
pkg/acpi/raw.go	67.85% <0.00%> (-10.72%)	⬇️
pkg/acpi/rsdp.go	38.46% <0.00%> (-10.26%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95ec011...a04ace4. Read the comment docs.

[ChriMarMe]

I will give it a try and start working on an integration test.

[ChriMarMe]

Wait. @rjoleary did you suggest I write a whole new docker image and make a custom qemu version run in the CI just to test ipmi? Because that's what I understand from the comment you left and the pdf you refered to.

[hugelgupf]

Could you do this PR without moving code around? It's a lot harder to review without knowing what you changed, and we'd have a clearer diff if no code was moved.

[hugelgupf]

Wait. @rjoleary did you suggest I write a whole new docker image and make a custom qemu version run in the CI just to test ipmi? Because that's what I understand from the comment you left and the pdf you refered to.

I will let @rjoleary clarify but an automated test with a real IPMI target to test against would be really really dope and really good use of time. Updating the QEMU in our existing docker image is not a lot of work.

[ChriMarMe]

Could you do this PR without moving code around? It's a lot harder to review without knowing what you changed, and we'd have a clearer diff if no code was moved.

This is just a new branch with the same commit as in #2182 This PR was reviewed by @rminnich until it reached a good state and then I squashed it into one commit.

[hugelgupf]

Unless @rjoleary or @rminnich agreed to or requested it, would you mind undoing the code moving? It makes it harder to see the diff of the ipmi code and what in the code changed. I don't disagree with the new split, actually, I would just rather see it in a different PR so that the diff is clean to read in this PR.

[hugelgupf]

(Ignore my comment, I see this is obsolete in favor of the VM test)

[ChriMarMe]

@johnnylinwiwynn Thank you very much.

This can be merged when #2274 is done.

[hugelgupf]

Unless @rjoleary or @rminnich agreed to or requested it, would you mind undoing the code moving? It makes it harder to see the diff of the ipmi code and what in the code changed. I don't disagree with the new split, actually, I would just rather see it in a different PR so that the diff is clean to read in this PR.

Since we do plan to merge this, would you mind addressing this?

[ChriMarMe]

Will do

[ChriMarMe]

#2281

[ChriMarMe]

Also this has to wait a little longer. Need to comply with the comment here first before we can get this in.
#2193 (comment)

[rminnich]

I'm approving it and it's up to you.

[rminnich]

Here is where this gets weird and why it's hard for me to say.

Will this test run on a plan 9 system, and use an imported /sys from a linux system? Or vice versa? That's extremely unlikely, but my world, not uncommon. But at that point, we might someday choose to support multiple GOOS in one binary ... but that's a very unlikely situation?

Anyway, I tend to trust Chris K on stuff like this, so I'd take his direction; or, since he does not feel too strongly, this may be a case where we should let you decide, since the big goal here is to have 9e make more decisions of this type as time goes on. Are you comfortable making this decision? I am fine with whatever you wish to do.

As you can see, it's kind of borderline.

[ChriMarMe]

Looking forward to the last review or will it be merged directly? Only the flying spaghetti monster knows!

[ChriMarMe]

Oh! Now I remember why I had the mock and the interface. I had to prevent using the dev.SendRequest method, because it calls f.RawConn, which is not possible with a normal file. The mock allowed me to get coverage for the most top level ipmi functions. I couldnt figure out how to create a file with the required properties to satisfy the call to SyscallConn. It was an additional layer of abstraction to actually run the functions without an ipmi interface available. Now we run the tests with the ipmi interface available, but the device does not support all features implemented. So we can't cover all functions. The problem with the file rawconn still persists in this environment, because as soon as I try to abstract the interface with a normal file it wont work.

This. Any idea might help. I'm a little clueless now. Is there a different solution of the RawConn problem and I dont see it?

I think we've worked it out, and we need to get this in. Sorry if that was the wrong thing to do.