Introduce setup and teardown fixtures

[Shwetha-Acharya]

With this change, multiple test modules triggered from testcases/mount can be run as individual tests complying to pytest standards. Also any number of new tests can be added efficiently on the mount.

Old approach:

• All the different tests are called from test_mount.py: https://github.com/samba-in-kubernetes/sit-test-cases/blob/main/testcases/mount/test_mount.py
• Test_mount.py creates a mount, the required tests are called and then test_mount.py cleans the mount.

New approach :

• This PR complies with the proposal to use fixtures in order to remove all the load on one single module: https://github.com/samba-in-kubernetes/sit-test-cases/pull/25#issuecomment-1721320096 https://github.com/samba-in-kubernetes/sit-test-cases/pull/25#issuecomment-1725320536
• The tests are no more called from test_mount.py.

• Setting up of mount and tearing down of mount is taken care by setup and teardown fixture which is defined in testcases/mount/conftest.py

  More information about fixtures: https://docs.pytest.org/en/6.2.x/fixture.html

• With this change, test files remain the same, but the initiator function starts with keyword test and the each test module starts with keyword 'test'

  That way the tests are called by pytest. Usage of setup and teardown fixture becomes easily possible. We no more need to to call the test function explictly.

  This also helps with easy debugging and better understanding of the code, as the tests will be failed at individual mount/IO test.

  In this case, pytest clearly points which test among test_mount_io.py, test_mount_dbm.py and test_mount_stress.py are failing. Can be very useful

  • when more and more tests are added
  • when we want to parse test results in sit-environment project

Fixes: #30

[Shwetha-Acharya]

This PR need few changes to be merged to pass the tests

[anoopcs9]

This PR need few changes to be merged to pass the tests

Please feel free to edit the description to add the keyword depends on #<PR.no> so that its visible in the list of checks.

[Shwetha-Acharya]

This PR need few changes to be merged to pass the tests

Please feel free to edit the description to add the keyword depends on #<PR.no> so that its visible in the list of checks.

Done!

[anoopcs9]

centos-ci/xfs seems to have failed . @Shwetha-Acharya can you check if its related to the proposed change?

[spuiuk]

The xfs tests are failing

testcases/mount/test_mount_dbm.py::test_check_dbm_consistency[192.168.123.10-share-xfs-default] FAILED [ 96%]
testcases/mount/test_mount_io.py::test_check_io_consistency[192.168.123.10-share-xfs-default] ERROR [100%]

...
E           Exception: Setup failed: Error mounting: ret 256 cmd: mount -t cifs -o username=test1,password=x //[192.168.123.10/share-xfs-default](http://192.168.123.10/share-xfs-default) /tmp//10073/mnt_0

conftest.py:32: Exception
---------------------------- Captured stderr setup -----------------------------
Couldn't chdir to /tmp//10073/mnt_0: No such file or directory

I believe that this is happening because of the teardown_mount() call by the first test. That deletes the temporary directory which was created for the test.

in conftest.py

tmp_root = testhelper.get_tmp_root()
mount_point = testhelper.get_tmp_mount_point(tmp_root)

will be called only once.

you should either create mount/unmount only once for the entire test or fetch a new tmp_root for every test.

[Shwetha-Acharya]

/retest all

[spuiuk]

/retest all

[spuiuk]

/retest centos-ci/xfs

[Shwetha-Acharya]

/retest centos-ci/xfs

[Shwetha-Acharya]

/retest centos-ci/xfs

[Shwetha-Acharya]

/retest all

[Shwetha-Acharya]

/retest all

[Shwetha-Acharya]

xfs build is successful:

    "=========================== short test summary info ============================",
    "PASSED testcases/containers/test_containers.py::test_containers[192.168.123.10-share-xfs-default-smallfiles]",
    "PASSED testcases/mount/test_mount_dbm.py::test_check_dbm_consistency[192.168.123.10-share-xfs-default]",
    "PASSED testcases/mount/test_mount_stress.py::test_check_mnt_stress[192.168.123.10-share-xfs-default]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.bench-oplock]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.connect]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.create]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.tcon]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.secleak]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.maximum_allowed]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.compound_find]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.mkdir]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.deny]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.lock]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.session]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.rw]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.rename]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.durable-open-disconnect]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.session-id]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.winattr]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.credits]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.sdread]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.openattr]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.sharemode]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.notify-inotify]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.read]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.check-sharemode]",
    "PASSED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-xfs-default-smb2.mangle]",
    "PASSED testcases/mount/test_mount_io.py::test_check_io_consistency[192.168.123.10-share-xfs-default]",
    "PASSED testcases/consistency/test_consistency.py::test_consistency[192.168.123.10-share-xfs-default]",
    "======================== 29 passed in 494.59s (0:08:14) ========================",
    "___________________________________ summary ____________________________________",
    "  pytest: commands succeeded",
    "  congratulations :)"
]

}

[Shwetha-Acharya]

@spuiuk regarding your proposal for removal of some helper functions and that change affecting this PR, I definitely agree there is much more scope for refactoring.

But I propose to keep the PRs independent, and small as possible (mostly addressing one issue) I propose taking this PR and working on other changes with next PR.

Else things can get messy and confusing, even for reviewers

[Shwetha-Acharya]

I can address next comments, if we are sure we want to go ahead with fixture approach or not

[spuiuk]

/retest all

[Shwetha-Acharya]

As per our discussion, went ahead with fixture approach and addressed all the comments and suggestions

[Shwetha-Acharya]

/retest all

[Shwetha-Acharya]

@spuiuk Please notice my recent reply on your comment of "run the test only on the first ip address instead of all the ip addresses."

I am yet to handle the case of premounted share in this PR, based on your reply to my comment, I can handle that case as well

[Shwetha-Acharya]

Some changes requested. Also, to complete this patch we need to also handle premounted shares.

@spuiuk please review this thread: https://github.com/samba-in-kubernetes/sit-test-cases/pull/38#discussion_r1411667741

I can handle premounted shares, on knowing how to go about addressing the comments in above thread

[Shwetha-Acharya]

@spuiuk sit environment is not adding any premounted share value inside test-info created by it:

Refer test-info.yaml on client from build artefacts of client in: https://jenkins-samba.apps.ocp.cloud.ci.centos.org/job/samba_cephfs.vfs-integration-test-cases/125/

private_interfaces:
  - 192.168.122.100
  - 192.168.122.101
  - 192.168.122.102

public_interfaces:
  - 192.168.123.10
  - 192.168.123.11
  - 192.168.123.12

exported_sharenames:
  - share-cephfs-default

test_users:
  -   password: x
      uid: '2001'
      username: test1
  -   password: x
      uid: '2002'
      username: test2

test_backend: cephf

FAILED testcases/smbtorture/test_smbtorture.py::test_smbtorture[share-cephfs-vfs-smb2.create]
SKIPPED [1] testcases/mount/test_mount_io.py:142: got empty parameter set ['test_dir'], function test_check_io_consistency_premounted at /root/sit-test-cases/testcases/mount/test_mount_io.py:141
SKIPPED [1] testcases/mount/test_mount_stress.py:68: got empty parameter set ['test_dir'], function test_dbm_consistency_premounted at /root/sit-test-cases/testcases/mount/test_mount_stress.py:67
SKIPPED [1] testcases/mount/test_mount_dbm.py:110: got empty parameter set ['test_dir'], function test_dbm_consistency_premounted at /root/sit-test-cases/testcases/mount/test_mount_dbm.py:109

Also the same code pass, when run locally by changing test-info.yml.example to test-info.yml (This is because the premounted share value is now present)

testcases/mount/test_mount_io.py::test_check_io_consistency_premounted[test_dir0] 

PASSED
testcases/mount/test_mount_io.py::test_check_io_consistency_premounted[test_dir1] 

PASSED

So, inorder to resolve this, I have added logic to ignore premounted share test, if there are no premounted shares in test-info fie.

This logic to skip is useful even if any user wants to manually run the tests without the value of premounted shares inside the test_info file

[spuiuk]

Please consider the following suggestions for a future PR. These aren't a result of the changes in this PR but will be nice to have for this test. 1) in _run_dbm_consistency_checks() and _check_io_consistency(), the mkdir call should have parents=False. I don't see why we should have this option set to True. A missing parent directory should be treated as an error. 2) _perform_file_operations() should perform the file operations in a sub directory to allow for easier clean up.

[spuiuk]

@spuiuk sit environment is not adding any premounted share value inside test-info created by it:

So, inorder to resolve this, I have added logic to ignore premounted share test, if there are no premounted shares in test-info fie.

This logic to skip is useful even if any user wants to manually run the tests without the value of premounted shares inside the test_info file

This is fine in my opinion. pytest skips the empty tests and it isn't considered an error. I have requested you to remove this logic from the code from my code review which I did earlier.

The premounted shares section is for use with Windows clients where we cannot use mount the shares for the client. In such cases, we need to consider pre-mounted locations to run the tests on. In such cases, we will see the opposite where Skipped messages will be seen on tests which expect to perform the cifs_mounts. eg: test_check_mnt_stress(), test_dbm_consistency(), test_check_io_consistency()

[Shwetha-Acharya]

Please consider the following suggestions for a future PR. These aren't a result of the changes in this PR but will be nice to have for this test.

1. in _run_dbm_consistency_checks() and _check_io_consistency(), the mkdir call should have parents=False. I don't see why we should have this option set to True. A missing parent directory should be treated as an error.
2. _perform_file_operations() should perform the file operations in a sub directory to allow for easier clean up.

I will take a look on it after this PR id merged. Else there can be a lot of merge conflicts.

[spuiuk]

/retest centos-ci/xfs