Noissue - Make comparable_xml safe against ambiguous tags

Noissue - Make comparable_xml safe against ambiguous tags

[tlucas, 2017-10-23 10:26:29]

Patch Set 1

 * Make comparable_xml safe against ambiguous element distributions

Hey there!

Vasily pointed this out in the previous review [1] and also provided the
majority
of the new code for comparable_xml().
This change also makes the output of XML-related assertion errors more readable.

Current error:
E       AssertionError: --- 
E         
E         +++ 
E         
E         @@ -18,2 +17,0 @@
E         
E        
-{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Package|None|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Applications|
E         
E        
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Application|
E          
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Extensions|
E            
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}Extension|
E              
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}AppExtension|
E              
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}Properties|
E                
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capabilities|
E                  
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capability|
E                       |None
E        
-{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Package|None|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Applications|
E         
E        
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Application|
E          
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Extensions|
E            
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}Extension|
E              
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}AppExtension|
E              
|__{http://schemas.microsoft.com/appx/manifest/uap/windows10/3}Properties|
E                
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capabilities|
E                  
|__{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capability|
E                       |None__Name:browserStorage
E       assert 5 == 0
E        +  where 5 = len(['--- \n', '+++ \n', '@@ -18,2 +17,0 @@\n',
'-{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Package...http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capability|\n
             |None__Name:browserStorage'])

New error:

E       AssertionError: --- 
E         
E         +++ 
E         
E         @@ -60,9 +59,0 @@
E         
E         -[
E        
-"{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capability", 
E         -"", 
E         -"", 
E         -[
E         -"Name=browserStorage"
E         -], 
E         -[]
E         -], 
E       assert 12 == 0
E        +  where 12 = len(['--- \n', '+++ \n', '@@ -60,9 +59,0 @@\n', '-[',
'-"{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Capability",
', '-"", ', ...])

Best,
Tristan

[1]https://codereview.adblockplus.org/29501558

[Vasily Kuznetsov, 2017-10-23 12:48:20]

Hi Tristan,

I have another small idea how to simplify this code given that you're feeding
the results to `comparable_json`.

Cheers,
Vasily

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
tests/test_packagerWebExt.py:259: attrs = tuple('{}={}'.format(k, v) for k, v in
elt.attrib.items())
Since you're doing a `comparable_json` later, we can just use `elt.attrib`
instead of this tuple comprehension thingy and `comparable_json` will handle
those dicts just fine, right?

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
tests/test_packagerWebExt.py:262: return
comparable_json(json.dumps(transform(ElementTree.fromstring(xml))))
Its kind of a pity that we're doing `json.dumps` here just to immediately
`json.loads` it in `comparable_json` but I guess it's just a few extra CPU
cycles so who cares.

[tlucas, 2017-10-23 13:21:37]

Hey there!

Patch Set 2:

 * Don't call dumps(loads(dumps()))
 * Simplifying transform()

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
tests/test_packagerWebExt.py:259: attrs = tuple('{}={}'.format(k, v) for k, v in
elt.attrib.items())
On 2017/10/23 12:48:20, Vasily Kuznetsov wrote:
> Since you're doing a `comparable_json` later, we can just use `elt.attrib`
> instead of this tuple comprehension thingy and `comparable_json` will handle
> those dicts just fine, right?

Good point - Done.

https://codereview.adblockplus.org/29586561/diff/29586562/tests/test_packager...
tests/test_packagerWebExt.py:262: return
comparable_json(json.dumps(transform(ElementTree.fromstring(xml))))
On 2017/10/23 12:48:20, Vasily Kuznetsov wrote:
> Its kind of a pity that we're doing `json.dumps` here just to immediately
> `json.loads` it in `comparable_json` but I guess it's just a few extra CPU
> cycles so who cares.

No, you are right - dumps(loads(dumps())) is definitely kind of ugly - Done.

[Vasily Kuznetsov, 2017-10-23 13:37:23]

LGTM