Issue 7059 - Modify fldiff so it can handle multiple files

tests/test_diff_script.py

Index: tests/test_diff_script.py
===================================================================
--- a/tests/test_diff_script.py
+++ b/tests/test_diff_script.py
@@ -20,8 +20,12 @@
 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
@@ -29,10 +33,24 @@
     """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
 
-    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
@@ -52,34 +70,58 @@
     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'