Issue 7059 - Modify fldiff so it can handle multiple files

tests/test_diff_script.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 """Functional tests for the diff script."""
 
 from __future__ import unicode_literals
 
 import pytest
 import subprocess
 import io
 import os
+import re
 
 from test_differ import BASE, LATEST
-from abp.filters.diff_script import main as diff_script
-
-BASE2 = BASE + '&adnet=\n'
 
 
 @pytest.fixture
 def rootdir(tmpdir):
-    """Directory with example filter lists."""
+    """Root directory for test files."""
     rootdir = tmpdir.join('root')
     rootdir.mkdir()
     rootdir.join('latest.txt').write_text(LATEST, encoding='utf8')
     return rootdir
 
 
 @pytest.fixture
-def archive_dir(tmpdir, rootdir):

[Vasily Kuznetsov, 2018/10/24 11:30:10]

You don't seem to be using tmpdir in this function. Probably no need to depend
on it.

[rhowell, 2018/10/26 21:42:21]

Done.

-    archive_dir = rootdir.join('archive')
-    archive_dir.mkdir()
-    archive_dir.join('list1.txt').write_text(BASE, encoding='utf8')
-    archive_dir.join('list2.txt').write_text(BASE2, encoding='utf8')
-    return archive_dir
+def archive_dir(rootdir):
+    return rootdir.mkdir('archive')
 
 
 @pytest.fixture
-def diff_dir(tmpdir, rootdir):

[Vasily Kuznetsov, 2018/10/24 11:30:09]

tmpdir also unused here, and the function could be made a bit more compact:

    return rootdir.mkdir('diff')

[rhowell, 2018/10/26 21:42:21]

Done.

-    diff_dir = rootdir.join('diff')
-    diff_dir.mkdir()
-    return diff_dir
+def diff_dir(rootdir):
+    return rootdir.mkdir('diff')
 
 
 @pytest.fixture
-def dstfile(tmpdir):
-    """Destination file for saving the diff output."""
-    return tmpdir.join('dst')
+def archived_files(archive_dir):
+    base2 = BASE + '&adnet=\n'
+    base2 = re.sub(r'! Version: \d+', '! Version: 112', base2)
+    archive_dir.join('list111.txt').write_text(BASE, encoding='utf8')
+    archive_dir.join('list112.txt').write_text(base2, encoding='utf8')
+    return [str(x) for x in archive_dir.listdir()]
+
+
+@pytest.fixture
+def base_no_version(archive_dir):
+    base = re.sub(r'! Version: \d+', '! ', BASE)
+    archive_dir.join('list113.txt').write_text(base, encoding='utf8')
+    return [str(x) for x in archive_dir.listdir()]
 
 
 def run_script(*args, **kw):
     """Run diff rendering script with given arguments and return its output."""
     cmd = ['fldiff'] + list(args)
 
     proc = subprocess.Popen(cmd, stderr=subprocess.PIPE,
                             stdout=subprocess.PIPE, stdin=subprocess.PIPE,
                             **kw)
     stdout, stderr = proc.communicate()

[Vasily Kuznetsov, 2018/10/24 11:30:09]

Since we can't really output to stdout anymore (because we're producing multiple
files), we can probably switch to using subprocess.run() here.

[rhowell, 2018/10/26 21:42:22]

Looks like subprocess.run() was added in Python 3.5, and won't be compatible
with the Python 2.7 tests [https://docs.python.org/3/library/subprocess.html].
The documentation recommends using subprocess.call() or .check_call() to remain
backwards-compatible. Unless we want to stop supporting Python 2?

[Vasily Kuznetsov, 2018/10/29 16:31:48]

Indeed, sorry, I forgot that we're also supporting Python 2 with this :/

     return proc.returncode, stderr.decode('utf-8'), stdout.decode('utf-8')
 
 
-def test_diff_with_dict(rootdir, archive_dir, diff_dir):
-    args = []
-    args.append('-l ' + str(rootdir.join('latest.txt')))
-    args.append('-o ' + str(diff_dir))
-    args.append(str(archive_dir.join('list1.txt')))

[Vasily Kuznetsov, 2018/10/24 11:30:09]

With the way we use `archive_dir` fixture, I'm thinking that perhaps it would be
better if it would be called `archived_files` and would return the list of
paths. Then we would just do `args.extend(archived_files)` here and in the other
test.

Two advantages of this approach are not duplicating the code that produces the
paths in several tests (we also have part of it in the fixture) and avoiding
hidden dependency on the list of files in the directory (any test that uses the
fixture also needs to have the knowledge what files are in the directory: the
fixture could just contain this information instead).

[rhowell, 2018/10/26 21:42:22]

Yeah, this way seems much cleaner.

-    args.append(str(archive_dir.join('list2.txt')))
-    diff_script(args)
-    for root, dirs, files in os.walk(str(diff_dir)):
-        for file in files:
-            with io.open(str(diff_dir.join(file)), encoding='utf-8') as dst:
-                result = dst.read()
-            assert '- &ad.vid=$~xmlhttprequest' in result
-            assert '+ &ad_channel=\xa3' in result
-            assert '! Version: 123' in result
+def test_diff_with_outfile(rootdir, archived_files, diff_dir):
+    run_script(str(rootdir.join('latest.txt')), '-o', str(diff_dir),
+               *archived_files)
+    assert len(diff_dir.listdir()) == 2
+    for file in diff_dir.visit():
+        with io.open(str(file), encoding='utf-8') as dst:
+            result = dst.read()
+        assert '- &ad.vid=$~xmlhttprequest' in result
+        assert '+ &ad_channel=\xa3' in result
+        assert '! Version: 123' in result
 
 
-def test_diff_with_outfile(rootdir, archive_dir, diff_dir):
-    run_script('-o ' + str(diff_dir),
-               '-l ' + str(rootdir.join('latest.txt')),
-               str(archive_dir.join('list1.txt')),
-               str(archive_dir.join('list2.txt')),
-               )
-    for root, dirs, files in os.walk(str(diff_dir)):
-        for file in files:
-            with io.open(str(diff_dir.join(file)), encoding='utf-8') as dst:
-                result = dst.read()
-            assert '- &ad.vid=$~xmlhttprequest' in result
-            assert '+ &ad_channel=\xa3' in result
-            assert '! Version: 123' in result
-
-
-def test_no_outfile(rootdir, archive_dir, diff_dir):

[Vasily Kuznetsov, 2018/10/24 11:30:09]

Since this mode of running doesn't really work with multiple base files, I think
it's the easiest to just stop supporting it (see also comments in the script).
We would not need this test then.

[rhowell, 2018/10/26 21:42:21]

Done.

-    _, _, out = run_script('-l ' + str(rootdir.join('latest.txt')),
-                           str(archive_dir.join('list1.txt')),
-                           str(archive_dir.join('list2.txt')),
-                           )
-    assert '[Adblock Plus Diff]' in out
+def test_diff_no_outfile(rootdir, archived_files):
+    os.chdir(str(rootdir))
+    run_script(str(rootdir.join('latest.txt')), *archived_files)
+    for file in ['diff111.txt', 'diff112.txt']:
+        with io.open(file, encoding='utf-8') as dst:
+            result = dst.read()
+        assert '- &ad.vid=$~xmlhttprequest' in result
+        assert '+ &ad_channel=\xa3' in result
+        assert '! Version: 123' in result
 
 
 def test_no_base_file(rootdir):
-    code, err, _ = run_script('-l ' + str(rootdir.join('latest.txt')))
-    assert code == 1
-    assert err == 'Archived lists are required\n'
+    code, err, _ = run_script(str(rootdir.join('latest.txt')))
+    assert code == 2
+    assert 'usage: fldiff' in err
 
 
-def test_no_latest_file(rootdir):
+def test_wrong_file(rootdir):
     code, err, _ = run_script(str(rootdir.join('base.txt')), 'wrong.txt')
     assert code == 1
-    assert err == 'The --latest list is required\n'
+    assert 'No such file or directory' in err
 
 
-def test_diff_to_self(rootdir):
-    _, _, out = run_script('-l ' + str(rootdir.join('latest.txt')),
-                           str(rootdir.join('latest.txt')))
-    assert out == '[Adblock Plus Diff]\n'
+def test_diff_to_self(rootdir, diff_dir):
+    run_script(str(rootdir.join('latest.txt')), '-o', str(diff_dir),
+               str(rootdir.join('latest.txt')))
+    assert len(diff_dir.listdir()) == 1
+    for file in diff_dir.visit():
+        with io.open(str(file), encoding='utf-8') as dst:
+            result = dst.read()
+        assert result == '[Adblock Plus Diff]\n'
+
+
+def test_no_version(rootdir, base_no_version):
+    code, err, _ = run_script(str(rootdir.join('latest.txt')), '-o',
+                              str(diff_dir), *base_no_version)
+    assert code == 1
+    assert 'Unable to find Version in ' in err
+
+
+def test_write_and_overwrite(rootdir, archived_files, diff_dir):
+    test_diff_with_outfile(rootdir, archived_files, diff_dir)
+    latest = re.sub(r'&act=ads_', '! ', BASE)
+    latest = latest + '&adurl=\n'
+    rootdir.join('latest.txt').write_text(latest, encoding='utf8')
+    run_script(str(rootdir.join('latest.txt')), '-o', str(diff_dir),
+               *archived_files)
+    assert len(diff_dir.listdir()) == 2
+    for file in diff_dir.visit():
+        with io.open(str(file), encoding='utf-8') as dst:
+            result = dst.read()
+        assert '- &act=ads_' in result
+        assert '+ &adurl=' in result
+        assert '- &ad.vid=$~xmlhttprequest' not in result