Issue 7230 - python-abp line2dict vectorization

tests/test_rpy.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
@@ skipping 131 matching lines @@
     """Test that the API result has the appropriate format.
 
     Checks for both keys and datatypes.
     """
     position = 'start' if line_type in {'header', 'metadata'} else 'body'
     data = line2dict(_TEST_EXAMPLES[line_type]['in'], position)
 
     assert data == _TEST_EXAMPLES[line_type]['out']
 
 
-def test_lines2dicts():
-    """Test that the API result has the appropriate format.
+def test_lines2dicts_start_mode():
+    """Test that the API returns the correct result in the appropriate format.
 
-    Checks for both keys and datatypes.
+    Checks for 'start' mode, which can handle headers and metadata.
     """
-    string_list = []
-    line_types = {b'EmptyLine': 'empty', b'Filter': 'filter_single',
-                  b'Comment': 'comment', b'Include': 'include'}
+    tests = [t for t in _TEST_EXAMPLES.values()]
+    ins = [ex['in'] for ex in tests]
+    outs = [ex['out'] for ex in tests]
 
-    for key in _TEST_EXAMPLES.keys():

[Vasily Kuznetsov, 2019/01/25 15:00:58]

This is equivalent to `for key in _TEST_EXAMPLES:`. I would recommend to either
use that and keep the rest of the code as is, or use the idiom:

    for key, value in _TEST_EXAMPLES.items():

This is a bit more "pythonic".

[rhowell, 2019/01/26 01:08:22]

Done.

-        if (_TEST_EXAMPLES[key]['out'][b'type'] != b'Header'

[Vasily Kuznetsov, 2019/01/25 15:00:58]

You can use `... not in {b'Header', b'Metadata'}`.

[rhowell, 2019/01/26 01:08:22]

Done.

-                and _TEST_EXAMPLES[key]['out'][b'type'] != b'Metadata'):
-            string_list.append(_TEST_EXAMPLES[key]['in'])
+    assert lines2dicts(ins, 'start') == outs
 
-    result_list = lines2dicts(string_list)
 
-    for line in result_list:
-        line_type = line_types[line[b'type']]
-        data = line2dict(_TEST_EXAMPLES[line_type]['in'], 'start')
+def test_lines2dicts_default():
+    """Test that the API returns the correct result in the appropriate format.
 
-        assert data == _TEST_EXAMPLES[line_type]['out']

[Vasily Kuznetsov, 2019/01/25 15:00:58]

So in the end we compare the output of
line2dict(_TEST_EXAMPLES[line_type]['in']) to _TEST_EXAMPLES[line_type]['out']
-- this seems like a test of line2dict with an innovative way of looping over
some part of _TEST_EXAMPLES. But we are not doing much checking of the output of
lines2dicts().

A better approach would be to first produce the list of all inputs and outputs:

    tests = [t for t in _TEST_EXAMPLES.values()
             if t[b'type'] not in {b'Header', b'Metadata'}]
    ins = [t['in'] for ex in tests]
    outs = [t['out'] for ex in tests]

and then we could just compare the output of `lines2dicts(ins)` to `outs`.

    assert lines2dicts(string_list, 'start') 

[rhowell, 2019/01/26 01:08:22]

Good idea, this test makes much more sense. I was trying to avoid something like
this because Python2 doesn't preserve the order of dicts, so the resulting lists
have the dicts in different orders, but the tests are passing every time on
Python2.7 so now I'm confused. Normally, lists that are in different order are
not considered equal.

E.g.

--> 179     assert lines2dicts(ins) == outs

ipdb> lines2dicts(ins)
[{'target': 'www.test.py/filtelist.txt', 'type': 'Include'}, {'selector':
{'type': 'css', 'value': 'div#ad1'}, 'action': 'hide', 'options': [('domain',
[('foo.com', True), ('bar.com', True)])], 'text': 'foo.com,bar.com##div#ad1',
'type': 'Filter'}, {'type': 'EmptyLine'}, {'type': 'Comment', 'text':
'Comment'}, {'selector': {'type': 'css', 'value': 'div#ad1'}, 'action': 'hide',
'options': [('domain', [('foo.com', True)])], 'text': 'foo.com##div#ad1',
'type': 'Filter'}]

ipdb> outs
[{'target': 'www.test.py/filtelist.txt', 'type': 'Include'}, {'selector':
{'value': 'div#ad1', 'type': 'css'}, 'action': 'hide', 'type': 'Filter',
'options': [('domain', [('foo.com', True), ('bar.com', True)])], 'text':
'foo.com,bar.com##div#ad1'}, {'type': 'EmptyLine'}, {'type': 'Comment', 'text':
'Comment'}, {'selector': {'value': 'div#ad1', 'type': 'css'}, 'action': 'hide',
'type': 'Filter', 'options': [('domain', [('foo.com', True)])], 'text':
'foo.com##div#ad1'}]

ipdb> lines2dicts(ins) == outs
True

But:

$ python2
Python 2.7.12 (default, Nov 12 2018, 14:36:49) 
[GCC 5.4.0 20160609] on linux2
False

[Vasily Kuznetsov, 2019/01/28 17:47:36]

This happens because the proposed code doesn't depend on the order of iteration
over a dict. The first line picks the tests that we are going to run and saves
them in a python list (`tests`). This way the order stays the same when we use
it to produce the lists of inputs and outputs.

[rhowell, 2019/01/29 01:44:47]

Ah, right, makes sense. I ran into this issue when I thought I read
`lines2dict`, and it was returning a dict. But now that it returns a list, the
order it preserved. Got it, thanks for the clarification!

+    By default, lines2dicts() does not correctly parse headers and metadata.
+    """
+    tests = [t for t in _TEST_EXAMPLES.values()
+             if t['out'][b'type'] not in {b'Header', b'Metadata'}]
+    ins = [ex['in'] for ex in tests]
+    outs = [ex['out'] for ex in tests]
+
+    assert lines2dicts(ins) == outs