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
 
 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."""
     rootdir = tmpdir.join('root')
     rootdir.mkdir()
-    rootdir.join('base.txt').write_text(BASE, encoding='utf8')
     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
+
+
+@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
+
+
+@pytest.fixture
 def dstfile(tmpdir):
     """Destination file for saving the diff output."""
     return tmpdir.join('dst')
 
 
 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_outfile(rootdir, dstfile):
-    run_script(str(rootdir.join('base.txt')),
-               str(rootdir.join('latest.txt')),
-               str(dstfile))
-    with io.open(str(dstfile), encoding='utf-8') as dst:
-        result = dst.read()
-    assert '+ &ad_channel=\xa3' in result
+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_no_outfile(rootdir):
-    _, _, out = run_script(str(rootdir.join('base.txt')),
-                           str(rootdir.join('latest.txt')))
+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_no_base_file(rootdir):
-    code, err, _ = run_script('wrong.txt', str(rootdir.join('latest.txt')))
+    code, err, _ = run_script('-l ' + str(rootdir.join('latest.txt')))
     assert code == 1
-    assert 'No such file or directory' in err
+    assert err == 'Archived lists are required\n'
 
 
 def test_no_latest_file(rootdir):
     code, err, _ = run_script(str(rootdir.join('base.txt')), 'wrong.txt')
     assert code == 1
-    assert 'No such file or directory' in err
+    assert err == 'The --latest list is required\n'
 
 
 def test_diff_to_self(rootdir):
-    _, _, out = run_script(str(rootdir.join('latest.txt')),
+    _, _, out = run_script('-l ' + str(rootdir.join('latest.txt')),
                            str(rootdir.join('latest.txt')))
     assert out == '[Adblock Plus Diff]\n'