Make xattr dependency conditional, add stub xattr library #228
Conversation
|
We should discuss testing requirements for the stub library and whether it makes sense to test it or if the file should be ignored. |
|
Ignoring redefinitions of |
There was a problem hiding this comment.
This looks good! If it's not too difficult, I think it make sense to add tests confirming that when the stubbed library is swapped in, the filesystem tests still pass.
The tests right now don't actually confirm much for us, though they do get test coverage numbers up. Testing the filesystem implementation with the stubs would be more representative of the real-world situation in which the stubs are used, and would therefore be a better set of tests in terms of confirming for us that this change behaves the way we expect.
I'm not sure if it would be possible to test using the stubs in the filesystem tests without adding steps to the GitHub Actions workflow. These steps would uninstall xattr from the Docker container and re-run the filesystem tests. There may be a tricky way to do it with pytest, though.
Long story short, I'd spend a little while (but not too long!) trying to add some better tests. If it proves too difficult or unwieldy, we won't let it block this PR, and I'll merge it.
@whyitfor @EdwardLarson may want to weigh in on this as well.
I am currently modifying the tests to confirm the return types of the stub functions match the type of the real |
The changes you describe would fix types, but still are more like unit tests. I'm proposing something more like integration or functional tests to confirm not just that the While the changes you're making are important, I think they're complementary to what I'm suggesting. |
|
Got it. In that case I think the cleanest way to handle this would be, as you suggested, to somehow rerun the filesystem tests after uninstalling |
ofrak_core/test_ofrak/components/test_filesystem_without_xattr.py
Outdated
Show resolved
Hide resolved
|
|
||
| .PHONY: serial | ||
| serial: | ||
| $(PYTHON) -m pytest -n0 -m "serial" test_ofrak --cov=ofrak --cov-report=term-missing | ||
| fun-coverage |
There was a problem hiding this comment.
Am I understanding correctly that the "serial" tests need to be run in a specified order?
If so, this is not ideal -- each "test" should be standalone, and any setup or teardown for it should be handled by a fixture (and not be a side-effect of a previously run test).
There was a problem hiding this comment.
The order of the serial tests does not matter. What matters is that the two tests are not run in parallel, since one test requires the real xattr library be installed, while the other requires it be uninstalled. So to guarantee that they are not run in parallel I have set them to run in serial.
| import logging | ||
|
|
||
|
|
||
| LOGGER = logging.getLogger(__name__) |
| subprocess.run(["pip", "uninstall", "xattr", "-y"]) | ||
| cwd = os.getcwd() | ||
| os.chdir(os.path.dirname(__file__)) | ||
| result = subprocess.run(["pytest", "./test_filesystem_component.py"]) | ||
| assert result.returncode == 0 | ||
| os.chdir(cwd) | ||
| subprocess.run(["pip", "install", "xattr"]) |
There was a problem hiding this comment.
- I wonder if there is another way to do this that does not require running this test in a subprocess (is there a way to monkeypatch this)
- Point 1 aside, the uninstall/install steps here are not part of the test. They should really be extracted into a fixture.
| @pytest.fixture(scope="session") | ||
| def move_to_test_directory(): | ||
| current_directory = os.getcwd() | ||
| os.chdir(EXAMPLE_DIRECTORY) | ||
| yield | ||
| os.chdir(current_directory) |
There was a problem hiding this comment.
Is this fixture needed here?
|
|
||
|
|
||
| async def test_get(xattr_stub_fixture, xattr_real_fixture, caplog): | ||
| xattr_real_fixture.set("user.foo", b"bar") |
There was a problem hiding this comment.
This creates a side effect -- the xattr for the target file is set here, but no cleanup happens. Test setup/teardown should all be happening in a fixture, to ensure no side effects.
|
|
||
|
|
||
| async def test_set(xattr_stub_fixture, xattr_real_fixture, caplog): | ||
| value = xattr_stub_fixture.set(None, None) |
There was a problem hiding this comment.
It is confusing here (and later on) that the stub fixture is getting passed different arguments than the real fixture, since it is unclear what is being tested.
My understanding for this and other tests in this file is, that the test is the following:
Run the method on stub and real xattr with same arguments, and validate that the return type matches
Making the arguments the same will make this intention clearer.
| value = xattr_stub.getxattr(None, None) | ||
| assert "Function getxattr" in caplog.text | ||
| assert type(value) == type( | ||
| xattr.getxattr(os.path.join(EXAMPLE_DIRECTORY, "README.md"), "user.foo") |
There was a problem hiding this comment.
This assumes that the xattr "user.foo" is already set. What happens if it isn't set?
One sentence summary of this PR (This should go in the CHANGELOG!)
Make xattr dependent on platform, add stub library that warns users without xattr that extended attributes are not supported on their platform.
Link to Related Issue(s)
#227
Please describe the changes in your request.
Make
xattrdependency optional based on platform. Add stub library that shadows the standardxattrlibrary's functionality, and any function calls toxattrare replaced with warnings that extended attributes are not supported on their platform. Fall back to stub library ifxattrnot found.Anyone you think should look at this, specifically?