Skip to content

test: fix meson test() executable definitions#333

Merged
igaw merged 1 commit intolinux-nvme:masterfrom
jk-ozlabs:fixes
Apr 5, 2022
Merged

test: fix meson test() executable definitions#333
igaw merged 1 commit intolinux-nvme:masterfrom
jk-ozlabs:fixes

Conversation

@jk-ozlabs
Copy link
Copy Markdown
Collaborator

Currently, all of the meson tests are defined to just run the single
main test.

Instead, we should be defining each of the tests to use the appropriate
executable().

Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

The register test is failing as it expects a specific NVMe device to be available on the CI system:

stdout:
open /sys/class/nvme/nvme10/device/resource0
stderr:
failed to open /sys/class/nvme/nvme10/device/resource0

Is there a preferred approach there? Should these tests be able to be be run on arbitrary CI, or should we remove the test() definitions?

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 5, 2022

When we initially added the tests to the test target, the assumption was these tests are able to run as standalone tests. But most of these tests they are written to run against real hardware.

I was playing with some ideas how to improve the testing. One thing I'd like to change is to move the tests which run against real hardware into it's own target. So that the test target is running just unit tests. For the other type of tests we need to have to setup a proper environment first. This is probably out of scope for the test target.

So in short, yes, I agree with you we should drop the hardware tests from the test target for now. They don't test a lot if they don't run as root anyway. And in the case of nvme-cli they even do write test on the real hardware. I don't think this is what people want :)

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 5, 2022

BTW, the Python tests are fine as unit tests. These just test if the Python is able to load the binding.

Currently, the tests under test/ require intractions with actual
hardware, which isn't available under CI.

[Additionally, the test() functions are mostly incorrect, as they'll all
just repeat the same test.c test]

This test drops the test() defitinitons, but leaves the executable()s
available for developer use, and adds a comment to indicate what we're
doing.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

OK, so if I'm understanding things correctly: we should drop the tests that require actual hardware from the test target; sounds reasonable. This will make the coverage reports somewhat worse though :)

And yes, I'll leave the python tests as-is.

Updated branch coming soon.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 5, 2022

Yes, the coverage tests look a bit sad indeed. This is something we should improve. Haven't really figured out what the best strategy here is. I welcome any input :)

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

I'm writing a few tests for the NVMe-MI branch now, which don't require hardware. Those just have a fake test transport for the actual communication with the device, which can respond according to the test's requirements.

We could do something similar for the MI core, where we have a fake ioctl() implementation, perhaps?

I'll push the new mi branch (for PR #76 ) soon, and we can see if that approach would be useful elsewhere...

@igaw igaw merged commit 20b6c2d into linux-nvme:master Apr 5, 2022
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 5, 2022

Cool, looking forward to your update. As previously sad, I haven't really looked into the details yet but I agree if we can get a simple environment setup for testing then let's try to get this working. For fully end-to-end test with nvme-cli we could harness blktests as @ChaitanayaKulkarni has suggested.

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

OK, done - the test-related change is up at e43e644

@eli-schwartz
Copy link
Copy Markdown
Contributor

And in the case of nvme-cli they even do write test on the real hardware. I don't think this is what people want :)

If tests are potentially destructive, then it makes sense to not run them in the default testing target.

On the other hand, if there are non-destructive tests which nevertheless need to run on real hardware -- maybe the solution is to just teach the tests to emit a SKIP status: https://mesonbuild.com/Unit-tests.html#skipped-tests-and-hard-errors

Then the tests still run, and people with real hardware can get a complete view of the test status, but if you don't have the hardware everything still passes but with an informational "some tests were skipped" effect.

@jk-ozlabs jk-ozlabs deleted the fixes branch April 6, 2022 02:28
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.

3 participants