unify: unified_patches order fix #71

mihaibivol · 2016-05-17T11:48:17Z

Fixes the way patches are chosen to be part of the final result.
Adds regression tests for the added fix.

Signed-off-by: Mihai Bivol miha.bivol@cern.ch

mihaibivol · 2016-05-17T11:48:37Z

coveralls · 2016-05-17T11:57:27Z

Coverage decreased (-0.3%) to 99.735% when pulling 2fdb4a0 on mihaibivol:fix_unify into 5258111 on inveniosoftware:master.

coveralls · 2016-05-17T12:10:02Z

Coverage remained the same at 100.0% when pulling 74f7bee on mihaibivol:fix_unify into 5258111 on inveniosoftware:master.

mihaibivol · 2016-05-17T12:15:58Z

dictdiffer/unify.py

            if conflict:
-                if not conflict.handled:
-                    conflict.handled = True


I removed this check since the same conflict can be checked twice:

on its first_patch

on its second_patch

Since we check below that the patch we are trying to apply is either first_patch or second_patch, given the conflict resolution, the second time the conflict is brought up from self._index it would always be different than the patch we are checking against.

patches = [fp1, sp1, sp2] conflicts = [(fp1,sp1), (fp1,sp2)] _index = { fp1: (fp1, sp2) sp1: (fp1, sp1), sp2: (fp1, sp2), } # Try to take first: # fp1 will point to (fp1, sp2) and its resolution is fp1 so we append. # sp1 will point to (fp1, sp1) but the conflict resolves to fp1. # sp2 will point to (fp1, sp2) but the conflict resolves to fp1. # -- We will never find fp1 in the patch list. # -- Every conflict that resolves to fp1 will never be matched with anything else in the list. unified = [fp1] # Try to take second: # fp1 will point to (fp1, sp2) but the conflict resolves to sp2, so we skip it. # sp1 will point to (fp1, sp1) and its resolution is sp1 so we append. # sp2 will point to (fp1, sp2) and its resolution is sp2 so we append. unified = [sp1, sp2]

Rough demonstration of algo:

We always append in the correct order

For any patch p in sorted patches

It never appeared before in sorted_patches

It will never appear afterwards in sorted_patches

If it points to a conflict:

Conflicts resolves to p and from 1st and 2nd bullet no other p1exists in sorted_patches so that p == p1. For appending a patch patch.take_patch() should be equal to p. Therefore we won't find another patch that satisfies this condition so there's no need to check about handling a conflict twice.

Conflict does not resolve to p but to p1 -> we will append p1 when it occurs in the list (if we didn't already)

jirikuncar · 2016-05-17T13:14:26Z

tests/test_merge.py

@@ -43,6 +44,41 @@ def take_first(conflict, _, __, ___):
        self.assertEqual(m.unified_patches,
                         [('change', 'changeme', ('Jo', 'Joe'))])

+    def test_continue_run_multiple_conflicts_per_patch(self):
+        lca = {'foo': [{'x': [1, 2, 3]}, {'y': [2, 3, 4]}]}


Can you try to make simpler test data and remove duplicated code from two tests?

def test_continue_run_multiple_conflicts_per_path(self): lca = {'first': [1, 2]} first = {'first': [1, 2, 3]} second = {'second': 2} expected = [ ('f', {'first': [1, 2, 3], 'second': 2}), ('s', {'second': 2}), ] for resolution, result in expected: m = Merger(lca, first, second, {}) try: m.run() except UnresolvedConflictsException as e: m.continue_run([resolution for _ in e.content]) self.assertEqual(patch(m.unified_patches, lca), result)

The simpler test case does not trigger the problem that I was fixing. We need nested objects for this test to fail before the fix, so I'll keep the large test fixtures.

Also, when regression testing the bug it made sense to have two separate tests for each case.

Also, when regression testing the bug it made sense to have two separate tests for each case.

@mihaibivol since we are using pytest, you can use @pytest.mark.parameterize to keep the results of each run visible.

coveralls · 2016-05-17T13:19:01Z

Coverage remained the same at 100.0% when pulling 757d86a on mihaibivol:fix_unify into 5258111 on inveniosoftware:master.

coveralls · 2016-05-18T08:10:06Z

Coverage remained the same at 100.0% when pulling 4defa07 on mihaibivol:fix_unify into 5258111 on inveniosoftware:master.

jirikuncar · 2016-05-18T09:13:35Z

tests/test_merge.py

+        first = {'foo': [{'x': 1}, {'y': 2}, {'z': 4}]}
+        second = {'bar': 'baz'}
+
+        expected = {


If you use list of tuples then you don't need following dict access expected[resolution] (moreover it's easier to use pytest.mark.parameterize).

This tests inherits from unittest.TestCase so that's why parametrize does not work (SO).
I'll just change to items() iteration and I'll leave it as is for the time being.

The follow-up solutions for this would either be:

Require nose-parametrize for parametrizing unitest.TestCase test methods

Port everything to pytest

I should have say "easier to port to use pytest.mark.parameterize.

coveralls · 2016-05-18T11:05:02Z

Coverage remained the same at 100.0% when pulling 0d7e6c1 on mihaibivol:fix_unify into 5258111 on inveniosoftware:master.

jirikuncar · 2016-05-18T13:08:44Z

@mvesper can you please have a look at these changes?

@mihaibivol please keep the PR up-to-date with the master branch until it gets integrated. Thanks

coveralls · 2016-05-18T13:21:01Z

Coverage remained the same at 100.0% when pulling b148b48 on mihaibivol:fix_unify into 7f5d303 on inveniosoftware:master.

mvesper · 2016-05-18T13:22:41Z

@jirikuncar Mihai and I discussed the issue when he found the bug. The changes and regression test seem reasonable.

jirikuncar · 2016-05-18T13:40:15Z

@mihaibivol @mvesper Thank you both!

Last thing I will ask @mihaibivol to amend commit message by adding FIX label to the first bullet point. Shall we mark removal of property Conflict.handled as backward INCOMPATIBLE change?

After you amend the commit message you can run kwalitee prepare release v0.5.0.. to see the new RELEASE-NOTES.rst (pip install -e git+https://github.com/inveniosoftware/kwalitee.git#egg=kwalitee).

mihaibivol · 2016-05-18T14:02:26Z

I don't seem to make kwalitee work due to some poor version of libgit2-dev in my package manager, so I just amended my commit.

coveralls · 2016-05-18T14:05:57Z

Coverage remained the same at 100.0% when pulling 0655e4f on mihaibivol:fix_unify into 7f5d303 on inveniosoftware:master.

jirikuncar · 2016-05-18T14:20:19Z

* FIX Fixes the way patches are chosen to be part of the final result.

Can you be more descriptive so people can understand this change in RELEASE-NOTES?

during conflict resolution
final result -> unified patch?
keep max. line length of bullet points < 72 chars.

pip install -e git+https://github.com/inveniosoftware/kwalitee.git#egg=kwalitee[gitpython]

* FIX Fixes order in which handled conflicts are unified so that the Merger's unified_patches can be always appplied. * Adds regression tests for the added fix. * Removes conflict.handled field. Signed-off-by: Mihai Bivol <miha.bivol@cern.ch>

coveralls · 2016-05-18T17:21:59Z

Coverage remained the same at 100.0% when pulling 707b459 on mihaibivol:fix_unify into 7f5d303 on inveniosoftware:master.

mihaibivol · 2016-05-18T22:08:23Z

New features

Adds support for comparing mutable mappings, sequences and sets from
collections.abs module. (diff: support for collections.Mapping comparison #67)

Improved features

Updates package structure, sorts imports and runs doctests.

Bug fixes

Fixes order in which handled conflicts are unified so that the
Merger's unified_patches can be always appplied.

mihaibivol force-pushed the fix_unify branch from 2fdb4a0 to 74f7bee Compare May 17, 2016 12:07

mihaibivol reviewed May 17, 2016
View reviewed changes

jirikuncar added this to the v0.5.1 milestone May 17, 2016

jirikuncar assigned jirikuncar and mvesper and unassigned jirikuncar May 17, 2016

jirikuncar reviewed May 17, 2016
View reviewed changes

mihaibivol force-pushed the fix_unify branch from 74f7bee to 757d86a Compare May 17, 2016 13:16

mihaibivol force-pushed the fix_unify branch from 757d86a to 4defa07 Compare May 18, 2016 08:08

jirikuncar reviewed May 18, 2016
View reviewed changes

mihaibivol force-pushed the fix_unify branch from 4defa07 to 0d7e6c1 Compare May 18, 2016 11:01

mihaibivol force-pushed the fix_unify branch from 0d7e6c1 to b148b48 Compare May 18, 2016 13:18

jirikuncar assigned mihaibivol and unassigned mvesper May 18, 2016

mihaibivol force-pushed the fix_unify branch from b148b48 to 0655e4f Compare May 18, 2016 14:00

unify: unified_patches order fix

707b459

* FIX Fixes order in which handled conflicts are unified so that the Merger's unified_patches can be always appplied. * Adds regression tests for the added fix. * Removes conflict.handled field. Signed-off-by: Mihai Bivol <miha.bivol@cern.ch>

mihaibivol force-pushed the fix_unify branch from 0655e4f to 707b459 Compare May 18, 2016 16:38

jirikuncar merged commit e637f35 into inveniosoftware:master May 19, 2016

jirikuncar assigned jirikuncar and unassigned mihaibivol May 19, 2016

mihaibivol deleted the fix_unify branch May 19, 2016 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unify: unified_patches order fix #71

unify: unified_patches order fix #71

mihaibivol commented May 17, 2016

mihaibivol commented May 17, 2016

coveralls commented May 17, 2016 •

edited

coveralls commented May 17, 2016 •

edited

mihaibivol May 17, 2016 •

edited

jirikuncar May 17, 2016 •

edited

mihaibivol May 18, 2016

jirikuncar May 18, 2016 •

edited

coveralls commented May 17, 2016 •

edited

coveralls commented May 18, 2016 •

edited

jirikuncar May 18, 2016

mihaibivol May 18, 2016

jirikuncar May 18, 2016

coveralls commented May 18, 2016 •

edited

jirikuncar commented May 18, 2016

coveralls commented May 18, 2016 •

edited

mvesper commented May 18, 2016

jirikuncar commented May 18, 2016

mihaibivol commented May 18, 2016

coveralls commented May 18, 2016 •

edited

jirikuncar commented May 18, 2016 •

edited

coveralls commented May 18, 2016 •

edited

mihaibivol commented May 18, 2016 •

edited

unify: unified_patches order fix #71

unify: unified_patches order fix #71

Conversation

mihaibivol commented May 17, 2016

mihaibivol commented May 17, 2016

coveralls commented May 17, 2016 • edited

coveralls commented May 17, 2016 • edited

mihaibivol May 17, 2016 • edited

Choose a reason for hiding this comment

jirikuncar May 17, 2016 • edited

Choose a reason for hiding this comment

mihaibivol May 18, 2016

Choose a reason for hiding this comment

jirikuncar May 18, 2016 • edited

Choose a reason for hiding this comment

coveralls commented May 17, 2016 • edited

coveralls commented May 18, 2016 • edited

jirikuncar May 18, 2016

Choose a reason for hiding this comment

mihaibivol May 18, 2016

Choose a reason for hiding this comment

jirikuncar May 18, 2016

Choose a reason for hiding this comment

coveralls commented May 18, 2016 • edited

jirikuncar commented May 18, 2016

coveralls commented May 18, 2016 • edited

mvesper commented May 18, 2016

jirikuncar commented May 18, 2016

mihaibivol commented May 18, 2016

coveralls commented May 18, 2016 • edited

jirikuncar commented May 18, 2016 • edited

coveralls commented May 18, 2016 • edited

mihaibivol commented May 18, 2016 • edited

New features

Improved features

Bug fixes

coveralls commented May 17, 2016 •

edited

coveralls commented May 17, 2016 •

edited

mihaibivol May 17, 2016 •

edited

jirikuncar May 17, 2016 •

edited

jirikuncar May 18, 2016 •

edited

coveralls commented May 17, 2016 •

edited

coveralls commented May 18, 2016 •

edited

coveralls commented May 18, 2016 •

edited

coveralls commented May 18, 2016 •

edited

coveralls commented May 18, 2016 •

edited

jirikuncar commented May 18, 2016 •

edited

coveralls commented May 18, 2016 •

edited

mihaibivol commented May 18, 2016 •

edited