Skip to content

Added test for test_util to ensure file absence when open for reading #1794

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

Closed
wants to merge 3 commits into from

Conversation

ilyas829
Copy link

@ilyas829 ilyas829 commented Oct 2, 2024

  • I have added a pytest function to ensure file absence when open for reading.
  • It basically for below purpose
  1. Creates a file
  2. Open the file for reading and
  3. Ensures that the file is removed
  • Please review my PR and merge it
  • also suggest your valuable feedback.

@ilyas829 ilyas829 mentioned this pull request Oct 2, 2024
@dirkmueller
Copy link
Member

@ilyas829 thanks for your contribution. could you please run "poetry run ruff format" before submitting ? you could amend the commit.

since you're only adding testcases, is the only purpose to increase test coverage or did you find a real issue somewhere?

@ilyas829
Copy link
Author

ilyas829 commented Oct 2, 2024

Thank you for reviewing my test coverage codes.. Yes the purpose of appending test cases were to increase test coverage but I din not found any issue as i wrote and tested these cases, but now I am moving up in the hierarchy and find errors or bugs in the test_user, test_services and so on.
After I complete the test cases, I am planning to make a test automation framework using python + selenium that will also help us to find bugs like link breakage, accessibility etc. (If you approve this I can start working on this after completion of test files).
Now I have added formatting to my previous branches too, As I have just got started I had to make several branches to avoid my confusion please approve that too.
Thanks again
Waiting for your suggestions and feedback .

@dirkmueller
Copy link
Member

Thanks. basically your new test is a duplicate of https://github.com/SUSE/BCI-dockerfile-generator/pull/1794/files#diff-0dcbadaa6c70afb6fea041dec04bb89c3cc7fe201fb7c3185a669efa49769f28R10-R16 . the fact that you can read a file after creating it is not super useful given that the purpose of this unit test is to validate that "ensure_absent" works.

Given that you confirmed that this isn't testing for a real issue, I would actually not be in favor of adding this test. if you can find a combination of file / directory / permission creation where ensure_absent breaks and you write a test for that, that would be awesome as a contribution.

@ilyas829
Copy link
Author

ilyas829 commented Oct 2, 2024

Perfect, 1st of all Thank you for describing the required test case. Yes we can do that.

  • To create a scenario where ensure_absent could potentially break, we need to consider edge cases that involve:

  • File permissions: Setting restrictive file or directory permissions, which could prevent deletion.

  • Special file types: Testing with special files like sockets, device files, or symbolic links with specific permissions.

  • One realistic edge case could involve restricted permissions on a directory or file, where ensure_absent is unable to remove it due to lack of write or execute permissions.

  • Edge Case: Directory with No Write Permissions

  • In this test case, we can create a directory with no write permissions and attempt to delete it using ensure_absent. This should raise a permission error, as the lack of write permissions will prevent the directory from being removed.

The code for the same is just pushed and please review and suggest your valuable feedback

@ilyas829
Copy link
Author

ilyas829 commented Oct 4, 2024

Hey @dirkmueller please can you check as it this condition is working for me..

@dirkmueller
Copy link
Member

@dcermak should we fix ensure_absent to handle permission errors like this rather than raising an exception?

os.chmod(path, 0o500)

# Try to remove the directory - should raise a PermissionError or OSError
with pytest.raises((PermissionError, OSError)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work if the tests are executed as root, root can do anything to a directory.

@alexandrevicenzi
Copy link
Member

I am planning to make a test automation framework using python + selenium that will also help us to find bugs like link breakage, accessibility etc. (If you approve this I can start working on this after completion of test files).

Why? This does not make any sense. This project is not a web application, there's no reason to use Selenium to test anything.

  • To create a scenario where ensure_absent could potentially break, we need to consider edge cases that involve:
  • File permissions: Setting restrictive file or directory permissions, which could prevent deletion.
  • Special file types: Testing with special files like sockets, device files, or symbolic links with specific permissions.
  • One realistic edge case could involve restricted permissions on a directory or file, where ensure_absent is unable to remove it due to lack of write or execute permissions.
  • Edge Case: Directory with No Write Permissions

Honestly, there's no reason to test for most of these scenarios. ensure_absent is used in one single place in the code.

It will never attempt to remove a socket or device, and if it did that is completely wrong.
It always has permission to remove the folder, because the folder that it tries to remove is created by the running user.

@alexandrevicenzi
Copy link
Member

@dcermak should we fix ensure_absent to handle permission errors like this rather than raising an exception?

@dirkmueller the folder that ensure_absent tries to remove 1 should not have permission issues because the folder is created by the running user 2.

I don't see a reason the change the current behavior of this function.

@dcermak
Copy link
Collaborator

dcermak commented Oct 7, 2024

ensure_absent is just a utility function that cleans up temporary files for the staging bot, the files are created beforehand by the current user and testing whether the function fails with EPERM is not really helpful, as this is not guaranteed behavior.

Thank you for your effort, but we don't want a test for a scenario that we do not wish to support.

@dcermak dcermak closed this Oct 7, 2024
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.

4 participants