dotbot icon indicating copy to clipboard operation
dotbot copied to clipboard

Support testing on Windows

Open kurtmckee opened this issue 2 years ago • 10 comments

This issue tracks development for testing on Windows.

This type of ticket typically generates a lot of traffic, so please only contribute to the technical discussion.

Targets

  • [x] Testing on Windows locally
  • [x] Testing on Windows in CI

Bonus targets

  • [x] Testing on Cygwin locally
  • [x] Testing on MacOS in CI

Implementation

The test environment will migrate from bash scripts to Python unit tests.

Open questions

1. Targets

@anishathalye Are the above targets acceptable to you? Are there others that need to be added?

2. Old Python versions

There currently appears to be no problem testing against Python 2.7 and higher on Linux and Cygwin, but only CPython 3.8 and higher can be supported on Windows due to CPython and PyPy symlink limitations.

~To facilitate testing in Python, it might be impossible to test Python versions that are unsupported by the PSF and most Python libraries. For example, Python 2.7 through 3.4 might not be feasible test targets. This does not necessarily mean that dotbot has to drop support for those older Python versions, but it might not be possible to test them.~

~@anishathalye, do I have your okay to proceed even if this is one of the outcomes?~

kurtmckee avatar Apr 13 '22 13:04 kurtmckee

Re high-level goals, that sounds great. If we can add support for testing on Windows locally, in CI, or both, that would be great. For testing on Windows, to have proper isolation without a lot of work, it might be easiest if the tests are run in a (Windows) VM, like they are with the current Linux VM testing implementation.

Re old Python versions: as long as the new testing setup doesn't remove any tests (/ OS / platform combos) that we currently run, that seems fine. I.e. adding Windows testing but only Python 3.6+ is fine, but removing Python 2.7 testing for Linux is not good.

Re implementation: why migrate from bash scripts to Python unit tests? I like that the bash scripts test Dotbot end-to-end, including the command-line interface, instead of testing internal functionality. Windows can run bash these days, even in GitHub Actions CI (with shell: bash).

anishathalye avatar Apr 13 '22 21:04 anishathalye

Re goals: I would ditch the VM requirement and just run the tests using pyfakefs to mock out filesystem isolation.

Re old Python versions: there's no reason to rip out the existing test work. I expected to recreate the tests so they run on any platform, and add code coverage so it's clear what's getting tested.

Re implementation: I don't use bash on Windows, so targeting bash for Windows doesn't help me ensure dotbot works well on my system (which, ultimately, is why I'm offering to do this 😀 ). I would mock sys.argv during testing and call dotbot.cli.main(). I don't currently think the CLI interface would go untested.

kurtmckee avatar Apr 13 '22 22:04 kurtmckee

I've pushed a branch that so far only demonstrates testing on all supported platforms.

Running the tests uses standard practices:

# Set up and activate a virtual environment, update pip,
# then install the project and dependencies.
python -m venv .venv
source .venv/bin/activate  # Powershell: "& .venv\Scripts\Activate.ps1"
python -m pip install -U pip setuptools wheel
python -m pip install -e .[dev]

# Run the tests.
tox

No useful tests are implemented yet, it's just a demonstration that the test dependencies and environments can be set up, and that my local development environment is set up to test every Python version dotbot supports.

Expect git history changes with that branch.

kurtmckee avatar Apr 14 '22 13:04 kurtmckee

Ok, that seems fine to me, I'm happy to incorporate additional tests that are cross-platform — that only improves testing coverage.

How good is pyfakefs? I don't have previous experience with it. If the mock doesn't accurately reflect how the underlying filesystem works (especially the Windows symlink weirdness), we might be lulled into a false sense of security and actually have a broken implementation. This is one reason why the original testing infrastructure actually runs Dotbot on top of the real fs and uses the really heavyweight isolation technique of VMs.

anishathalye avatar Apr 14 '22 20:04 anishathalye

Re pyfakefs: I've used it several times in the past and had been under the impression I could use it in a "chroot" capacity as well; I saw an add_real_directory() method back when I was last using it. I read the API docs, though, and that's not what it does.

It looks like pyfakefs will get us entirely off the filesystem, so I'll look at mocking things so we're still touching the actual filesystem, but nothing outside of a temporary directory.

kurtmckee avatar Apr 14 '22 23:04 kurtmckee

The current setup is already kind of like that, if you export HOME=/tmp/.... The issue is that this doesn't protect against the possibility of a bug in Dotbot where it messes with something elsewhere in the filesystem.

anishathalye avatar Apr 14 '22 23:04 anishathalye

I'm making good progress, though CPython/PyPy compatibility with Windows and symlinks is poor until recent CPython versions:

  • Python 2.7 doesn't even have an os.symlink() function.
  • PyPy 2 and 3 both raise NotImplementedError when os.symlink() is called.
  • Windows only introduced non-admin symlink creation in the last 2 or 3 years (and you have to enable Windows 10 Developer Mode first), and older Windows Pythons don't use the new Windows API flag that circumvents admin privilege requirements.
  • Functions like os.path.realpath() on Windows Pythons <= 3.7 don't resolve NTFS directory junctions (the equivalent of a directory symlink) to their targets.
  • Functions like shutil.rmtree() on Windows Pythons <= 3.7 can't delete NTFS directory junctions correctly.

Python >= 3.8 on Windows work great, however, and I've made significant progress with pytest fixtures that restrict write access to a temporary directory. Even if there's a bug in dotbot, it still has to call core functions like open() and os.symlink() to affect the system, and I've created a lengthy list of functions that are now tightly controlled to check the paths and destinations to ensure they're rooted in the temporary directory.

I'm now just translating the bash tests over to Python. It's slow going, but I've got about 14 test files (total of 29 individual tests) translated so far.

When I'm done, I'll retest everything with the bash test suite using Vagrant, but I'll then test on Linux and Cygwin to confirm the Python tests work well there, too.

kurtmckee avatar Apr 22 '22 12:04 kurtmckee

That sounds like fantastic progress (and quite the adventure!). So for Windows users, we'll only support Python 3.8+ and Windows 10+? That seems fine to me. From an outsider's perspective, Windows's UNIX-y support and developer support seem to have gotten a lot better in the recent past.

anishathalye avatar Apr 22 '22 13:04 anishathalye

Thanks! I agree that dotbot should only support Python 3.8+ on Windows. Technically users could still run dotbot as admins and expect basic link configurations to work (like ~/.git --> ${DOTFILES}/git), but I wouldn't recommend offering support for more than what you can test in CI. :sunglasses:

kurtmckee avatar Apr 22 '22 13:04 kurtmckee

Current status:

✔ All bash-based tests have been migrated to Python. ✔ Tests are passing on Windows (locally and on CI), including isort and black linters. ✔ As a bonus, HTML coverage reports are generated when running locally to show what code is actually being executed. The HTML reports are written to the standard htmlcov/ directory. ✔ As a bonus, the tests are passing on Cygwin. ✔ As a bonus, the tests are running against MacOS in CI.

Left to do:

❌ 3 tests are failing on MacOS. These are all associated with MacOS using a symlink from /tmp to /private/tmp, which affects the unit test framework, not the actual dotbot functionality. I'll resolve this before opening a PR.

An example run can be viewed here:

https://github.com/kurtmckee/dotbot/actions/runs/2332475013

kurtmckee avatar May 16 '22 14:05 kurtmckee