[ENH] make as many tox tests work under windows as possible #347

ericzolf · 2020-05-03T07:29:20Z

Is your feature request related to a problem? Please describe.

We don't have a test suite under Windows and this has led to regressions in the move from 1.2/1.3 to 2.0 and might lead again to regressions. Not all the tests can work, but most should work.

Describe the solution you'd like

I've started a branch https://github.com/rdiff-backup/rdiff-backup/tree/ericzolf-make-tox-work-under-win but it requires more work and I fear much more work, so if someone wants to take over, be my guest!

ericzolf · 2020-07-28T05:22:24Z

The approach is rather simple, even if perhaps tedious:

create your development environment under Windows is the first step (obviously). There is help under tools/windows even if you just read through the Ansible playbooks to understand what's being done.
then copy tox.ini to tox_win.ini and adapt to the Windows needs (already started in the branch)
then adapt step by step each test in the "commands_pre" and in the "command" section until the whole file works.

My suggestion would be to first comment out all tests and then uncomment them step by step, making a Pull Request for each few steps in order to avoid having one big PR at the end with a major review, while still having value to each PR. Feel free to create a "draft/WIP" PR if you need help, it makes review and discussion around the code rather easy.

t9t · 2020-08-03T14:35:53Z

So far I'm stumbling into some issues where some bytes APIs don't work on Windows and/or some APIs don't accept bytes, etc.

For example in commontest.py os.getenvb is used, which is not available on Windows, see:

So now I'm working my way through just changing some things one by one to try and get more and more working.

But in parallel (as not a very experienced Python developer) I would like to enquire why bytes types are used rather than strings, so that I won't go changing them all to strings in places where it's not appropriate?

t9t · 2020-08-03T19:48:12Z

I have addressed a few baby steps in #444. I can continue to address more and more, but as a next step I will try to fix up the tox_win.ini from your branch in a way that all tests are commented-out by default except the ones that run fine, and then the others can be addressed on by one.

Unfortunately I was unable to set up vagrant properly on my Linux machine (Ubuntu 18.04 LTS) and I don't want to go through all the effort of setting up all the Ansible stuff on my Windows environment (on which Docker won't run anyway, because it's in a VirtualBox VM). So I cannot verify anything using the automated tests on Windows so will continue to keep doing it manually using tox in my Python env.

I already foresee a lot of problems with the usage of bytes types all over the place, though. It looks like a bunch of Python APIs using bytes are not available on Windows, only offering one with strings. As I mentioned in a comment above I don't really understand why bytes are used instead of strings, so I want to understand that first before I start changing those instances with strings.

Another point I could already identify but not yet address is rdiff being called, which is obviously not available on Windows. I'm not sure yet what a good approach would be for that.

ericzolf · 2020-08-04T05:47:29Z

I have addressed a few baby steps in #444. I can continue to address more and more, but as a next step I will try to fix up the tox_win.ini from your branch in a way that all tests are commented-out by default except the ones that run fine, and then the others can be addressed on by one.

Good approach, I'm supporting it and reviewed the PR. We have an issue with Travis CI but we'll get it fixed.

Small hint: have a look at the @unittest.skipXXX decorators to avoid spending too much time on tests which can not be run on Windows (something like @unittest.skipIf(os.name == "nt") - we already have a few examples in the testing directory).

Unfortunately I was unable to set up vagrant properly on my Linux machine (Ubuntu 18.04 LTS) and I don't want to go through all the effort of setting up all the Ansible stuff on my Windows environment (on which Docker won't run anyway, because it's in a VirtualBox VM). So I cannot verify anything using the automated tests on Windows so will continue to keep doing it manually using tox in my Python env.

OK, I can help on setting up Vagrant but it can indeed be a beast.

I already foresee a lot of problems with the usage of bytes types all over the place, though. It looks like a bunch of Python APIs using bytes are not available on Windows, only offering one with strings. As I mentioned in a comment above I don't really understand why bytes are used instead of strings, so I want to understand that first before I start changing those instances with strings.

It's a longer story, I'll try to make it short:

str under Python2 were used all over the place and needed to be replaced by Python3-str or bytes.
bytes is the recommendation for Linux/UNIX paths and str/UTF-8 for Windows paths. That's not helpful for a cross-platform tool...
because one strong requirement expressed especially by @ikus060 was to support also non-UTF-8 charsets and even broken encodings, the only remaining solution was to use bytes all over the place, with some drawbacks indeed.
to add to the picture, the Python team wanted to deprecate bytes-paths in Windows starting with 3.7 but then reversed their decision with 3.8, so it's a bit messy right now but should get better once we can use Python 3.8 under Windows (i.e. once PyInstaller will support it).

Another point I could already identify but not yet address is rdiff being called, which is obviously not available on Windows. I'm not sure yet what a good approach would be for that.

I'm a bit short in time but we're aware of the issue and are in discussion with @dbaarda, the maintainer of librsync about this. Check #67 and #207. Don't expect an easy/quick fix hence.

* #347: make Myrm() in commontest.py cross-platform * #347: do not use getenvb in commontest.py as it is not cross-platform * #347: do not read from /dev/urandom directly

t9t · 2020-08-07T09:34:14Z

@ericzolf I am actually a bit confused why this particular issue is closed. :)

I have created the following pull request based on your tox_win.ini and temporarily commented out each of the failing tests: #447

After that, I could go through each test one by one.

ericzolf · 2020-08-08T18:50:26Z

Sorry, I can't remember having closed the issue so I think I've just pressed the wrong button. The approach agreed previously is still valid, no issue. Sorry for the confusion. Re-opening.

* making tox work under Windows * #347: tox_win: commented failing tests and included deps * statisticstest also runs fine on Windows

ericzolf · 2020-08-14T13:19:43Z

@t9t I tried to make tox_win.ini working under Windows but it fails on the statistics test, does it ring a bell to you? Same code version works under Linux...

vagrant@WIN-4SPEID5E7R8 C:\Users\vagrant\Develop\rdiff-backup>tox -c tox_win.ini

GLOB sdist-make: C:\Users\vagrant\Develop\rdiff-backup\setup.py
py37 inst-nodeps: C:\Users\vagrant\Develop\rdiff-backup\.tox\.tmp\package\1\rdif
f-backup-2.0.6.dev14+g667f75c.zip
py37 installed: altgraph==0.17,future==0.18.2,importlib-metadata==1.7.0,pefile==
2019.4.18,pyinstaller==4.0,pyinstaller-hooks-contrib==2020.7,pywin32==228,pywin3
2-ctypes==0.2.0,rdiff-backup @ file:///C:/Users/vagrant/Develop/rdiff-backup/.to
x/.tmp/package/1/rdiff-backup-2.0.6.dev14%2Bg667f75c.zip,zipp==3.1.0
py37 run-test-pre: PYTHONHASHSEED='794'
py37 run-test-pre: commands[0] | cmd /c set
NUMBER_OF_PROCESSORS=2
PROMPT=$P$G
SYSTEMDRIVE=C:
TEMP=C:\Users\vagrant\AppData\Local\Temp
PATH=C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\Scripts;c:\python37\lib\sit
e-packages\pywin32_system32;C:\Python37\Scripts\;C:\Python37\;C:\Windows\system3
2;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0
\;C:\Windows\System32\OpenSSH\;C:\ProgramData\chocolatey\bin;C:\Program Files\Op
enSSH;C:\Program Files\Microsoft VS Code\bin;C:\Program Files\Git\cmd;C:\Windows
\system32\config\systemprofile\AppData\Local\Microsoft\WindowsApps;C:\Users\vagr
ant\AppData\Local\Microsoft\WindowsApps;
COMSPEC=C:\Windows\system32\cmd.exe
PROCESSOR_ARCHITECTURE=AMD64
SYSTEMROOT=C:\Windows
PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.PY;.PYW
TMP=C:\Users\vagrant\AppData\Local\Temp
LIBRSYNC_DIR=C:\Users\vagrant\Develop\librsync.d
TOX_WORK_DIR=C:\Users\vagrant\Develop\rdiff-backup\.tox
USERPROFILE=C:\Users\vagrant
PYTHONHASHSEED=794
TOX_ENV_NAME=py37
TOX_ENV_DIR=C:\Users\vagrant\Develop\rdiff-backup\.tox\py37
TOX_PACKAGE=C:\Users\vagrant\Develop\rdiff-backup\.tox\.tmp\package\1\rdiff-back
up-2.0.6.dev14+g667f75c.zip
VIRTUAL_ENV=C:\Users\vagrant\Develop\rdiff-backup\.tox\py37
py37 run-test-pre: commands[1] | cmd /c copy '%LIBRSYNC_DIR%\bin\rsync.dll' 'C:\
Users\vagrant\Develop\rdiff-backup\.tox\py37\Scripts'
        1 file(s) copied.
py37 run-test-pre: commands[2] | python 'C:\Users\vagrant\Develop\rdiff-backup\.
tox\py37\Scripts\rdiff-backup' --version
rdiff-backup 2.0.6.dev14+g667f75c 
py37 run-test-pre: commands[3] | python testing/commontest.py 
testing/commontest.py:24: DeprecationWarning: The Windows bytes API has been dep
recated, use Unicode filenames instead
  old_test_dir = os.path.join(os.path.dirname(os.getcwdb()),
py37 run-test: commands[0] | python testing/ctest.py 
.... 
----------------------------------------------------------------------
Ran 4 tests in 0.001s

OK
py37 run-test: commands[1] | python testing/timetest.py
............ 
----------------------------------------------------------------------
Ran 12 tests in 2.142s

OK
py37 run-test: commands[2] | python testing/statisticstest.py 
Remote schema option ignored - no remote file descriptions. 
Remote schema option ignored - no remote file descriptions.
Exception '' raised of class '<class 'MemoryError'>':
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\robust.py", line 35, in check_common_error
    return function(*args)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\Rdiff.py", line 98, in patch_local
    return outrp.write_from_fileobj(patchfile)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\rpath.py", line 1462, in write_from_fileobj
    copyfileobj(fp, outfp)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\rpath.py", line 79, in copyfileobj
    inbuf = inputfp.read(blocksize)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\librsync.py", line 69, in read
    self._add_to_outbuf_once()
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\librsync.py", line 98, in _add_to_outbuf_once
    self.eof, len_inbuf_read, cycle_out = self.maker.cycle(self.inbuf)

Ec:\python37\lib\unittest\case.py:656: ResourceWarning: unclosed file <_io.Buffe
redWriter name=b'C:/Users/vagrant/Develop/rdiff-backup/.tox/py37/testfiles/outpu
t/subdir/rdiff-backup.tmp.10'>
  outcome.errors.clear()
ResourceWarning: Enable tracemalloc to get the object allocation traceback      
c:\python37\lib\unittest\case.py:656: ResourceWarning: unclosed file <_io.Buffer
edReader name=b'C:/Users/vagrant/Develop/rdiff-backup/.tox/py37/testfiles/output
/subdir/400K'>
  outcome.errors.clear()
ResourceWarning: Enable tracemalloc to get the object allocation traceback      
.......
======================================================================
ERROR: testStatistics (__main__.IncStatTest)
Test the writing of statistics
----------------------------------------------------------------------
Traceback (most recent call last):
  File "testing/statisticstest.py", line 194, in testStatistics
    time.time() + 1)
  File "C:\Users\vagrant\Develop\rdiff-backup\testing\commontest.py", line 169, 
in InternalBackup
    Main._action_backup(rpin, rpout)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\Main.py", line 445, in _action_backup
    backup.Mirror_and_increment(rpin, rpout, _incdir)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\backup.py", line 55, in Mirror_and_increment
    DestS.patch_and_increment(dest_rpath, source_diffiter, inc_rpath)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\backup.py", line 281, in patch_and_increment
    ITR(diff.index, diff)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\rorpiter.py", line 313, in __call__
    last_branch.fast_process(*args)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\backup.py", line 781, in fast_process
    if self._patch_to_temp(mirror_rp, diff_rorp, tf):
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\backup.py", line 617, in _patch_to_temp
    elif not self._patch_diff_to_temp(basis_rp, diff_rorp, new):
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\backup.py", line 669, in _patch_diff_to_temp
    self.error_handler, Rdiff.patch_local, (basis_rp, diff_rorp, new))
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\robust.py", line 35, in check_common_error
    return function(*args)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\Rdiff.py", line 98, in patch_local
    return outrp.write_from_fileobj(patchfile)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\rpath.py", line 1462, in write_from_fileobj
    copyfileobj(fp, outfp)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\rpath.py", line 79, in copyfileobj
    inbuf = inputfp.read(blocksize)
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\librsync.py", line 69, in read
    self._add_to_outbuf_once()
  File "C:\Users\vagrant\Develop\rdiff-backup\.tox\py37\lib\site-packages\rdiff_
backup\librsync.py", line 98, in _add_to_outbuf_once
    self.eof, len_inbuf_read, cycle_out = self.maker.cycle(self.inbuf)
MemoryError

----------------------------------------------------------------------
Ran 8 tests in 0.214s
 
FAILED (errors=1)
sys:1: ResourceWarning: unclosed file <_io.BufferedWriter name=b'C:/Users/vagran
t/Develop/rdiff-backup/.tox/py37/testfiles/output/rdiff-backup-data/backup.log'>
ERROR: InvocationError for command 'C:\Users\vagrant\Develop\rdiff-backup\.tox\p
y37\Scripts\python.EXE' testing/statisticstest.py (exited with code 1)

ericzolf · 2020-08-15T07:49:05Z

One small step towards understanding the above issue: it doesn't have anything to do with the tox setup, I can reproduce outside of the tox "cage".

ericzolf · 2020-08-15T08:35:01Z

It has even nothing to do with the testcase, I can reproduce the issue with the following snippet under Windows:

rdiff-backup ..\rdiff-backup_testfiles\stattest1 \temp\bla
rdiff-backup ..\rdiff-backup_testfiles\stattest2 \temp\bla

Or vice-versa, no difference: the first call succeeds, the 2nd one fails.

ericzolf · 2020-08-15T08:40:57Z

I moved the specific problem to a specific issue... let's focus here further on the test cases.

t9t · 2020-08-16T08:03:39Z

Before my absence, I ran each test individually to see what worked (and could uncomment in #447) and what didn't work yet and why. Broadly, the following issues exist:

Tests where a system call is performed for some general testing functionality (eg. cp -a). These could "just" be replaced with pure Python calls (although for example for cp -a specifically, there does not seem to be an equivalent functionality in Python, so this could be yet a little bit more complicated)
A handful of tests which use the rdiff program, for which there is no simple fix (mentioned earlier in this issue also)
Quite a lot of tests which call rdiff-backup as an external command. This doesn't work like that on Windows, because you can't just call any file as executable like on Linux: it has to be an actual executable (.exe) file. So probably the fastest work-around would be to do something like python.exe rdiff-backup ... given that the environment has been set up correctly (I was not able to fix it by only replacing the command with that). But I also wonder why in these tests, the app is actually called as an external command. Instead, they could also be changed to call the actual Python code from the tests (eg. rdiff_backup.Main.error_check_Main).
Some tests which hook into functionalities where OS differences are not taken into account, but should have been set up by the app code in an earlier stage; one example I found is where files with a : (eg. in timestamps) are produced, which are invalid on Windows (while the alternative, replacing : by - is compatible on all OSes). I had some trouble understanding what the best way was to address this, but also when working around that issue (eg. by setting a certain global setting) I bumped into one of the other issues (eg. rdiff-backup not being invoked properly).

There are also plenty of tests which failed where I wasn't able to identify the cause of the failure at first glance. It could even be that in some cases the test was actually working fine, but there might have been a bug in the code. Unfortunately, I had quite a hard time reading and understanding the actual rdiff-backup application code. And I must confess that this is also the point where I lost some motivation to work on this (this was also the moment when I realised that in fact I'm not such a big fan of Python at all 😂).

I think the most interesting next step for now is to figure out what to do with the tests that call rdiff-backup as an external system call, and address that issue. That would by fix a whole bunch of more tests, and also unblocks fixing issues in other tests.

ericzolf · 2020-08-16T13:52:35Z

I think the most interesting next step for now is to figure out what to do with the tests that call rdiff-backup as an external system call, and address that issue. That would by fix a whole bunch of more tests, and also unblocks fixing issues in other tests.

Hence I start to answer this one: I think I have already fixed the issue in my Vagrant setup. Just create a file called rdiff-backup.bat in the same directory where rdiff-backup is (I guess it's {envbindir}) with the following content (you might need to adapt to the tox context):

@ECHO OFF
REM simple wrapper script to call rdiff-backup from the repo
REM d=Disk, p=(dir)path, n=name(without extension), x=extension
python "%~dpn0" %*

The trick here is that Windows finds rdiff-backup.bat if you search for rdiff-backup but doesn't find rdiff-backup 🤷‍♂️

After that, you can follow different courses of action, or a mixture of those:

add "stupidly" the decorator @unittest.skipIf(sys.platform.startswith("win"), "This test doesn't work under Windows") to each test function which fails.
add a more elaborated decorator which actually checks the reason why the test fails and skips it accordingly. I have already a few examples, e.g. in rpathtest: @unittest.skipUnless(os.path.exists('/dev/tty2'), "Test requires /dev/tty2").
actually find a way to make the test work under Windows.

I'd personally, but it really depends on how you prefer to work, start with skipping (and hence identifying) all failing tests with decorators, either intelligently if the reason is obvious, or stupidly if it's too complicated. That could easily be a the subject of a single MR, because it'll be easy to review, only addition of decorators before failing test functions.

Then, in a second pass, test script after test script, remove the skipping decorators and make more and more tests work under Windows.

Does it make sense? Does it help?

Reference: https://docs.python.org/3/library/unittest.html#skipping-tests-and-expected-failures

ericzolf added enhancement help wanted labels May 3, 2020

t9t mentioned this issue Jul 27, 2020

[ENH] Document in which ways 2.x is incompatible with 1.x #436

Closed

t9t mentioned this issue Aug 3, 2020

[BUG] Windows build error: _librsyncmodule.obj : error LNK2001: unresolved external symbol rs_... #443

Closed

t9t mentioned this issue Aug 3, 2020

Better cross-platform compatibility in test files #444

Merged

ericzolf closed this as completed Aug 4, 2020

ericzolf reopened this Aug 8, 2020

ericzolf pushed a commit that referenced this issue Aug 9, 2020

tox_win.ini: commented failing tests and included deps (#447)

20f339e

* making tox work under Windows * #347: tox_win: commented failing tests and included deps * statisticstest also runs fine on Windows

ericzolf mentioned this issue Aug 14, 2020

Add Windows test playbook to Vagrant tools #450

Merged

ericzolf mentioned this issue Aug 15, 2020

Fail with MemoryError on Windows 64 bits using stattest #453

Closed

ericzolf mentioned this issue Aug 16, 2020

Make testing/resourceforktest.py work on OS/X #82

Open

ericzolf closed this as completed in 6a9b88d May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] make as many tox tests work under windows as possible #347

[ENH] make as many tox tests work under windows as possible #347

ericzolf commented May 3, 2020

ericzolf commented Jul 28, 2020

t9t commented Aug 3, 2020

t9t commented Aug 3, 2020

ericzolf commented Aug 4, 2020 •

edited

t9t commented Aug 7, 2020

ericzolf commented Aug 8, 2020

ericzolf commented Aug 14, 2020

ericzolf commented Aug 15, 2020

ericzolf commented Aug 15, 2020

ericzolf commented Aug 15, 2020

t9t commented Aug 16, 2020

ericzolf commented Aug 16, 2020

[ENH] make as many tox tests work under windows as possible #347

[ENH] make as many tox tests work under windows as possible #347

Comments

ericzolf commented May 3, 2020

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

ericzolf commented Jul 28, 2020

t9t commented Aug 3, 2020

t9t commented Aug 3, 2020

ericzolf commented Aug 4, 2020 • edited

t9t commented Aug 7, 2020

ericzolf commented Aug 8, 2020

ericzolf commented Aug 14, 2020

ericzolf commented Aug 15, 2020

ericzolf commented Aug 15, 2020

ericzolf commented Aug 15, 2020

t9t commented Aug 16, 2020

ericzolf commented Aug 16, 2020

ericzolf commented Aug 4, 2020 •

edited