dee

Merge lp://qastaging/~mhr3/dee/change-type-clear into lp://qastaging/dee

  1. change-type-clear
  2. Merge into trunk
Proposed by Michal Hruby
Status: Merged
Approved by: Didier Roche-Tolomelli
Approved revision: 312
Merged at revision: 310
Proposed branch: lp://qastaging/~mhr3/dee/change-type-clear
Merge into: lp://qastaging/dee
Diff against target: 380 lines (+245/-12)
4 files modified
dee/dee-shared-model.c (+103/-5)
tests/Makefile.am (+5/-0)
tests/model-helper-clear3add5.c (+65/-0)
tests/test-model-interactions.c (+72/-7)
To merge this branch: bzr merge lp://qastaging/~mhr3/dee/change-type-clear
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Review via email: mp+83018@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Overall it looks ok, and the tests all run. I also verified that the output of dbus-monitor "interface='com.canonical.Dee.Model'" looks as expected.

However, I'd still like to see an extra test for this: Testing a batched clear+add_rows transaction which is a very common scenario which how now been somewhat specialcased in the code.

review: Needs Fixing
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

I am not sure test_clear_add() tests what I was intending. Specifically what I was after was a scenario like:

 1) a model with N > 0 rows is synced between 2 parties (this looks ok in the branch)
 2) then the leader does a clear() and append() x M in the same transaction
 3) we test that the slave contains precisely the M expected rows

You seem to be testing something like this instead:

 1) a model with N > 0 rows is synced between 2 parties
 2) leader clears the model
 3) test that the slave model is cleared

review: Needs Fixing
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

(forgot to add - I think you need to introduce a new model helper to do this The Right Way (TM))

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

On the one!

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-dee/9/console reported an error when processing this lp:~mhr3/dee/change-type-clear branch.
Not merging it.

Revision history for this message
Unity Merger (unity-merger) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-dee/10/console reported an error when processing this lp:~mhr3/dee/change-type-clear branch.
Not merging it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: