Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Clean up the existing tests.py #286

Conversation

EchterAgo
Copy link

@EchterAgo EchterAgo commented Oct 11, 2023

This cleans up the code of tests.py and makes it use the new functions from utils.py. This also makes the test sleep much less, which is only still required before destroying the pool due to #282

@EchterAgo
Copy link
Author

Draft because I still need to make the driveletter test work. With the current code it just creates new pools with the same drive letter over and over again.

@andrewc12
Copy link

ZFS used to not reuse drive letters.

So the test was meant to try to repeatedly create and destroy pools one after the other so that if it ran out of drive letters it would fail.

It looks like you might be trying to assign drive letters yourself? From my brief glances.

@EchterAgo
Copy link
Author

EchterAgo commented Oct 12, 2023

ZFS used to not reuse drive letters.

So the test was meant to try to repeatedly create and destroy pools one after the other so that if it ran out of drive letters it would fail.

It looks like you might be trying to assign drive letters yourself? From my brief glances.

Yes, zfs_create does assign a driveletter by default. I want to add some option to not assign a driveletter. This is not a problem with the current test, but something I want to have working before merging the tests.py update.

@EchterAgo EchterAgo force-pushed the improve_tests_py branch 4 times, most recently from 7f02e78 to 64ac0dd Compare October 13, 2023 11:53
@EchterAgo
Copy link
Author

I rebased this on top of #288 so maybe this finally passes now?

@EchterAgo EchterAgo marked this pull request as ready for review October 13, 2023 12:20
@EchterAgo
Copy link
Author

Added another commit that removes Start-Sleep from test1

@andrewc12
Copy link

andrewc12 commented Oct 13, 2023

I only re-enabled that test because pytest 5 was crashing

I will straight up disable it after pytest5 passes

So you don't need to bother fixing it

@EchterAgo
Copy link
Author

@andrewc12 ok, removed.

@EchterAgo
Copy link
Author

Note that I still need to fix the drive letter test so it doesn't explicitly set a drive letter. I want to be able to detect what drive letter / mountpoint the newly created pool was assigned in create_zpool.

@EchterAgo EchterAgo force-pushed the improve_tests_py branch 6 times, most recently from 9b80b7c to 7b4d2ca Compare October 13, 2023 19:48
@EchterAgo
Copy link
Author

EchterAgo commented Oct 14, 2023

I just got this failure, not sure what happened:

2023-10-14 11:52:54,832 - tests.tests - INFO - ============================================================
2023-10-14 11:52:54,832 - tests.tests - INFO - Running test: snapshot no hang
2023-10-14 11:52:55,339 - tests.tests - INFO - Drive letters after testsn01 pool create: {'testsn01': WindowsPath('E:/')}
zunmount(testsn01,E:/) running
zunmount(testsn01,E:/) returns 0
Traceback (most recent call last):
  File "Y:\openzfs\contrib\windows\tests\tests.py", line 164, in <module>
    main()
  File "Y:\openzfs\contrib\windows\tests\tests.py", line 100, in main
    allocate_file(pool.mount_path / "test01.file", 1 * Size.KIB)
  File "Y:\openzfs\contrib\windows\tests\utils.py", line 332, in allocate_file
    with open(path, "wb") as f:
         ^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\test01.file'

I think the pool was actually mounted on a different drive letter than the test expected. But the drive letter is fixed isn't it? Maybe the previous unmount did not finish yet and the drive letter D was still in use? What happens if a driveletter is specified but unavailable?

@EchterAgo
Copy link
Author

Maybe we should also have a test to see if the drive letter is available again after unmount.

@EchterAgo
Copy link
Author

We should also probably test if the pool is actually mounted where we expect it on each test.

@EchterAgo EchterAgo force-pushed the improve_tests_py branch 2 times, most recently from c7fd1bd to 9c28d51 Compare October 14, 2023 06:17
@EchterAgo EchterAgo force-pushed the improve_tests_py branch 2 times, most recently from 51b2889 to ab30010 Compare October 14, 2023 06:43
@EchterAgo
Copy link
Author

I added the checks in zpool_create.

This cleans up the code of `tests.py` and makes it use the new functions
from `utils.py`. This also makes the test sleep much less, which is only
still required before destroying the pool due to
openzfsonwindows#282

Signed-off-by: Axel Gembe <[email protected]>
@andrewc12
Copy link

Maybe we should also have a test to see if the drive letter is available again after unmount.

We should also probably test if the pool is actually mounted where we expect it on each test.

Yeah these are both good ideas

@andrewc12 andrewc12 merged commit 5fcb34f into openzfsonwindows:zfs-Windows-2.2.0-release Oct 14, 2023
28 of 29 checks passed
@EchterAgo EchterAgo deleted the improve_tests_py branch October 15, 2023 01:05
@EchterAgo
Copy link
Author

FYI, I let this test loop for the last two days and only encountered the drive letter thing once. I still intend to fix it.

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.

2 participants