When using 4k disk and sector=512, we expect sanlock to fail with
SanlockException(EINVAL) when writing or reading.
Two tests are marked a expected failure:
Rephrase the part in yellow. Maybe you meant: "...marked with an expected..."?
It should be:
Two tests are marked as expected failure:
- write_resource() - not sure how sanlock can succeed with wrong sector
size - this may be a bug.
- read_resource_owners() - sanlock uses a fallback mechanism, hiding
wrong value from the user. I think we can make sanlock more strict.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
tests/python_test.py | 79 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/tests/python_test.py b/tests/python_test.py
index f0ecd06..aa1f7f5 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -99,10 +99,35 @@ def test_write_lockspace_4k(sanlock_daemon, align):
# Check that sanlock did not write after the lockspace area.
f.seek(align)
assert f.read(4096) == b"X" * 4096
Might be worth to define the 4K constant that in turn will use constants.KiB that comes with my PR.
I will add a proper constant, since we use it in multiple tests.
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
Suggestion to rephrase the reason text (added in bold): "Requires user specified SanLock path when using 4k block size"
The suggestion is wrong, and the original text is correct - the specified path is should be
a block device using 4k sector size or a file on a file system on such block device.
However the text does not make it clear that this was skipped because SANLOCK_4K_PATH
was not specified, I will add it to the message.
+def test_write_lockspace_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.write_lockspace(
+ "name", path, iotimeout=1, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
Suggestion to rephrase the reason text (added in bold): "Requires user specified SanLock path when using 4k block size"
There is no need to repeat the same comment again and again.
However this is my fault, repeating the mark again and again. We can define the mark like this once:
requires_sanlock_4k_path = pytest.mark.skipif(
"SANLOCK_4K_PATH" not in os.environ,
reason="Requires
SANLOCK_4K_PATH using 4k block size")
And then use the mark like this:
@requires_sanlock_4k_path
def test_foo():
+def test_read_lockspace_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
+
+ sanlock.write_lockspace("name", path, iotimeout=1, sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_lockspace(path, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
@@ -185,10 +210,64 @@ def test_write_resource_4k(sanlock_daemon, align):
# Check that sanlock did not write after the resource area.
Suggestion to change in the above comment "after" --> "beyond"
f.seek(align)
assert f.read(4096) == b"X" * 4096
Might be worth to define the 4K constant that in turn will use constants.KiB that comes with my PR.
There is no need to repeat the same comment again and again.
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
Suggestion to rephrase the reason text (added in bold): "Requires user specified SanLock path when using 4k block size"
+@pytest.mark.xfail(reason="need to investigate why the call succeed")
+def test_write_resource_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
+ disks = [(path, 0)]
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.write_resource(
+ "ls_name", "res_name", disks, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
Suggestion to rephrase the reason text (added in bold): "Requires user specified SanLock path when using 4k block size"
+def test_read_resource_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
+ disks = [(path, 0)]
+
+ sanlock.write_resource(
+ "ls_name",
+ "res_name",
+ disks,
+ align=ALIGNMENT_1M,
+ sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_resource(path, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
Suggestion to rephrase the reason text (added in bold): "Requires user specified SanLock path when using 4k block size"
+@pytest.mark.xfail(reason="fallback hides wrong value from caller")
+def test_read_resource_owners_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
+ disks = [(path, 0)]
+
+ sanlock.write_resource(
+ "ls_name",
+ "res_name",
+ disks,
+ align=ALIGNMENT_1M,
+ sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_resource_owners(
+ "ls_name", "res_name", disks, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
Forgot to comment about this?
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
--
2.17.2