Merge lp://qastaging/~kevin-wright-1/u1db-qt/synchronizer-merged-with-trunk-8-aug into lp://qastaging/u1db-qt

FAILED: Continuous integration, rev:113
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/1/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/1/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/1/rebuild

The code deals with id's being wrapped in {} in places. That's based on behavior of the core code; I think we need a newid() function and solve this in one place.

There's a few instances of the pattern "if (!query.exec()) qDebug" which should probably be using setError.

resetModel can be used in more places currently still doing its equivalent calls.

getValidTargets() and synchronizeTargets() hard-code HTML formatting - that's not good if we plan to see errors in console or XML and will make those cases harder to deal with. The messages should be predictable without using markup - if needed, it's still an option to use regex.
m_errors.append("<b><font color=\"red\">Database "+index_number+"</font></b>: F

FIXMEs that ought to be sorted out:
##KW## The two lists below are currently not used for anything
##KW## maybe this can be removed as it is replaced by revision_number

Unused code:
//targetDb->updateSyncLog(true, QString uid, QString generation, QString transaction_id);
//double target_replica_generation = replyMap["target_replica_generation"].toDouble();

This should probably say the real app name + " (U1Db-Qt)"
request.setRawHeader("User-Agent", "My app name v0.1");
request.setRawHeader("X-Custom-User-Agent", "My app name v0.1");

For good measure processDataFromRemoteServer might want to qDebug if an unexpected i.key() is seen?

This should not be writable, it's only set internally
Q_PROPERTY(QList<QString> errors READ getErrors WRITE setErrors NOTIFY errorsChanged)

"where u1db has been downloaded/branched"
This is very ambiguous and doesn't say where to obtain it. It should say
"where the u1db Python reference implemented has been downloaded from lp:u1db"

FAILED: Continuous integration, rev:116
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/2/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/2/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/2/rebuild

Den 08/08/2013 16:24, skrev Christian Dywan:
> Review: Needs Fixing
>
> The code deals with id's being wrapped in {} in places. That's based on behavior of the core code; I think we need a newid() function and solve this in one place.

Agreed. Will add.

> There's a few instances of the pattern "if (!query.exec()) qDebug" which should probably be using setError.

Agreed. Will modify.
>
> resetModel can be used in more places currently still doing its equivalent calls.
Not sure what this comment means.
> getValidTargets() and synchronizeTargets() hard-code HTML formatting - that's not good if we plan to see errors in console or XML and will make those cases harder to deal with. The messages should be predictable without using markup - if needed, it's still an option to use regex.
> m_errors.append("<b><font color=\"red\">Database "+index_number+"</font></b>: F

The type of errors here are for the benefit of app devs and app users at
the UI level, not console etc. The messages are for use in a QML
application as output from a sync transation to notify re: errors,
warnings and things that went according to plan.

An alternative approach, with broader and more flexible possibilities,
is to have the error information available in a model (QVariantMap) that
includes useful meta data and values (e.g. type of message, message
text, other?). In that way it can be available to app developers as well
as easily adapted for other purposes (e.g. log files, console output).

>
> FIXMEs that ought to be sorted out:
> ##KW## The two lists below are currently not used for anything
> ##KW## maybe this can be removed as it is replaced by revision_number

Both removed.

>
> Unused code:
> //targetDb->updateSyncLog(true, QString uid, QString generation, QString transaction_id);
Could not find this one. Perhaps it was deleted in another modification.
> //double target_replica_generation = replyMap["target_replica_generation"].toDouble();

This variable is probably needed in upcoming modifications, so it is
there but commented out so it does not produce an annoying warning at
compile time.

>
> This should probably say the real app name + " (U1Db-Qt)"
> request.setRawHeader("User-Agent", "My app name v0.1");
> request.setRawHeader("X-Custom-User-Agent", "My app name v0.1");

Changed to 'U1Db-Qt v1.0' on both lines. Those two lines may not even be
necessary, as the server may ignore them, but are kept there and posted
anyway for good measure.

>
> For good measure processDataFromRemoteServer might want to qDebug if an unexpected i.key() is seen?

Not sure it would be needed. If an unexpected key showed up it would be
ignored (as it should be).

>
> This should not be writable, it's only set internally
> Q_PROPERTY(QList<QString> errors READ getErrors WRITE setErrors NOTIFY errorsChanged)

Agreed. Will modify accordingly.

> "where u1db has been downloaded/branched"
> This is very ambiguous and doesn't say where to obtain it. It should say
> "where the u1db Python reference implemented has been downloaded from lp:u1db"
Changed.

FAILED: Continuous integration, rev:117
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/3/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/3/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/3/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/3/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/3/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/3/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/3/rebuild

> > getValidTargets() and synchronizeTargets() hard-code HTML formatting -
> that's not good if we plan to see errors in console or XML and will make those
> cases harder to deal with. The messages should be predictable without using
> markup - if needed, it's still an option to use regex.
> > m_errors.append("<b><font color=\"red\">Database
> "+index_number+"</font></b>: F
>
> The type of errors here are for the benefit of app devs and app users at
> the UI level, not console etc. The messages are for use in a QML
> application as output from a sync transation to notify re: errors,
> warnings and things that went according to plan.
>
> An alternative approach, with broader and more flexible possibilities,
> is to have the error information available in a model (QVariantMap) that
> includes useful meta data and values (e.g. type of message, message
> text, other?). In that way it can be available to app developers as well
> as easily adapted for other purposes (e.g. log files, console output).

Okay, I better see the reasoning now. And I think you're mixing two concepts into here, UI error messages and error handling API for letting developers distinguish errors. The user must not see verbatim key names. And the app developer needs a way to distinguish a failing sync from a bug in his code.

I had a thought in that direction in respect to Database::setError originally. SQL errors aren't interesting to anyone but U1Db-Qt developers - they need to be simple and pastable. The same is often true for U1Db-Qt errors. Errors about invalid id's or json are indeed relevant for developers - but only in the console. There's no need to "handle" them, wrong use of the API is to be fixed. One kind of error that we should have API for is: no space left. I apparently never filed a bug for that.

Now for sync, I'd still want the same console output for obvious mistakes like invalid URLs and wrong key names. That is what I was having in mind when I suggested to not have HTML. It's a one-time see the mistake and fix it and not for UI. And it is useful to always log if a connection failed in any way. For the developer's benefit we should have "signal syncFailed". That should suffice to prompt the user appropriately - the developer knows their target audience, what language to use etc.

> > For good measure processDataFromRemoteServer might want to qDebug if an
> unexpected i.key() is seen?
>
> Not sure it would be needed. If an unexpected key showed up it would be
> ignored (as it should be).

I should clarify: if an unexpected key turns up that's a bug in the code. Q_ASSERT is better I guess.

Den 08/08/2013 17:32, skrev Christian Dywan:
> Review: Needs Fixing
>
>
>>> For good measure processDataFromRemoteServer might want to qDebug if an
>> unexpected i.key() is seen?
>>
>> Not sure it would be needed. If an unexpected key showed up it would be
>> ignored (as it should be).
> I should clarify: if an unexpected key turns up that's a bug in the code. Q_ASSERT is better I guess.

In fact there are other key and value pairs that the server does return
that are not yet being used by the Synchronizer class.

Of course it is harmless to notify that there is more information than
expected (or in this case information that is expected but not being
used), and discounting the possibility that at some point in time there
might be a benefit to doing so would not be wise.

For the record though, in this particular case, if there were an
unexpected key (and associated value) that turns up it is because it was
information delivered directly from the server.

Kevin

Den 08/08/2013 17:32, skrev Christian Dywan:
> Review: Needs Fixing
>
>>> getValidTargets() and synchronizeTargets() hard-code HTML formatting -
>> that's not good if we plan to see errors in console or XML and will make those
>> cases harder to deal with. The messages should be predictable without using
>> markup - if needed, it's still an option to use regex.
>>> m_errors.append("<b><font color=\"red\">Database
>> "+index_number+"</font></b>: F
>>
>> The type of errors here are for the benefit of app devs and app users at
>> the UI level, not console etc. The messages are for use in a QML
>> application as output from a sync transation to notify re: errors,
>> warnings and things that went according to plan.
>>
>> An alternative approach, with broader and more flexible possibilities,
>> is to have the error information available in a model (QVariantMap) that
>> includes useful meta data and values (e.g. type of message, message
>> text, other?). In that way it can be available to app developers as well
>> as easily adapted for other purposes (e.g. log files, console output).
> Okay, I better see the reasoning now. And I think you're mixing two concepts into here, UI error messages and error handling API for letting developers distinguish errors. The user must not see verbatim key names. And the app developer needs a way to distinguish a failing sync from a bug in his code.

The messages can and should be modified. But the app developer does not
need to necessarily use the log/error/warning output from sync in its
raw form either. It should be up to the developer to determine how they
want to work with the data.

>
> I had a thought in that direction in respect to Database::setError originally. SQL errors aren't interesting to anyone but U1Db-Qt developers - they need to be simple and pastable. The same is often true for U1Db-Qt errors. Errors about invalid id's or json are indeed relevant for developers - but only in the console. There's no need to "handle" them, wrong use of the API is to be fixed. One kind of error that we should have API for is: no space left. I apparently never filed a bug for that.
>
> Now for sync, I'd still want the same console output for obvious mistakes like invalid URLs and wrong key names. That is what I was having in mind when I suggested to not have HTML. It's a one-time see the mistake and fix it and not for UI. And it is useful to always log if a connection failed in any way. For the developer's benefit we should have "signal syncFailed". That should suffice to prompt the user appropriately - the developer knows their target audience, what language to use etc.

Assuming too much about what the app developer wants, the endless
possible types of errors that might be encountered (e.g. faulty app
logic, user input variations, API problems), or who their audience is,
along rigid lines (and then use that logic for designing the API), may
be asking for trouble. What one developer to the next might want (or
need) could be different enough that "signal syncFailed" isn't enough.
This is why I suggested having a much more flexible and less
discriminating approach of making the data vailable using QVarientMap,...

> Assuming too much about what the app developer wants, the endless
> possible types of errors that might be encountered (e.g. faulty app
> logic, user input variations, API problems), or who their audience is,
> along rigid lines (and then use that logic for designing the API), may
> be asking for trouble. What one developer to the next might want (or
> need) could be different enough that "signal syncFailed" isn't enough.
> This is why I suggested having a much more flexible and less
> discriminating approach of making the data vailable using QVarientMap,
> with meta data included so u1db dev and app dev can use the information
> in ways they choose, for the ultimate benefit of the end user.
>
> The existing approach is indeed immature in that respect, but the
> suggestion of using a model (QVarientMap) moves beyond current
> limitations and is flexible enough for a broad set of use cases, some of
> which we cannot anticipate on our own, but without creating huge overhead.
>
> It is intended to provide a rich mechanism for developers to create
> robust applications for users that do more than simply give feedback
> that only says "fail". Additionally it can hopefully provide useful
> information for u1db-qt devs as well (who also might happen to build
> apps with the plugin...including examples).

syncFailed was an example only. And I stress free-form strings are not a basis for proper error handling API.

At this point I'd prefer to only use simple qDebug() calls and leave proper error API for a separate task. I created bugs for this: bug 1210448 and bug 1210450.

These are the doc errors I'm currently seeing with the latest changes. They need to be resolve before the branch can be merged:

./src/database.cpp:427: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/database.cpp:428: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/database.cpp:429: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:33: warning: Cannot find Synchronizer specified with \class in any header file
./src/synchronizer.cpp:156: warning: Cannot find Synchronizer::source specified with \property in any header file
./src/synchronizer.cpp:173: warning: Cannot find Synchronizer::targets specified with \property in any header file
./src/synchronizer.cpp:189: warning: Cannot find Synchronizer::synchronize specified with \property in any header file
./src/synchronizer.cpp:202: warning: Cannot find Synchronizer::errors specified with \property in any header file
./src/synchronizer.cpp:214: warning: Cannot find Synchronizer::errors specified with \property in any header file
./src/synchronizer.cpp:358: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:359: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:360: warning: Unknown command '\return'
./src/synchronizer.cpp:783: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:817: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:818: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/synchronizer.cpp:889: warning: Unknown command '\param'
 [Maybe you meant '\part'?]
./src/database.cpp:398: warning: Undocumented parameter doc_id in Database::getCurrentDocRevisionNumber()
./src/database.cpp:255: warning: No documentation for 'Database::getDocumentContents()'
./src/database.cpp:324: warning: Undocumented parameter doc_id in Database::getNextDocRevisionNumber()
./src/database.cpp:818: warning: No documentation for 'Database::getSyncLogInfo()'
./src/database.cpp:792: warning: No documentation for 'Database::listTransactionsSince()'
./src/database.cpp:619: warning: No documentation for 'Database::resetModel()'
./src/database.cpp:463: warning: No documentation for 'Database::updateDocRevisionNumber()'
./src/database.cpp:425: warning: Undocumented parameter transaction_id in Database::updateSyncLog()
./src/database.cpp:425: warning: Undocumented parameter generation in Database::updateSyncLog()
./src/database.cpp:425: warning: Undocumented parameter insert in Database::updateSyncLog()
./src/database.cpp:425: warning: Undocumented parameter uid in Database::updateSyncLog()
./src/query.cpp:161: warning: No documentation for 'Query::resetModel()'

[snip]
>
> The existing approach is indeed immature in that respect, but the
> suggestion of using a model (QVarientMap) moves beyond current
> limitations and is flexible enough for a broad set of use cases, some of
> which we cannot anticipate on our own, but without creating huge overhead.
>
> It is intended to provide a rich mechanism for developers to create
> robust applications for users that do more than simply give feedback
> that only says "fail". Additionally it can hopefully provide useful
> information for u1db-qt devs as well (who also might happen to build
> apps with the plugin...including examples).
> syncFailed was an example only. And I stress free-form strings are not a basis for proper error handling API.
>
> At this point I'd prefer to only use simple qDebug() calls and leave proper error API for a separate task. I created bugs for this: bug 1210448 and bug 1210450.

I modified things already.

I renamed it sync_output instead of errors (since not everything was an
error), and changed it so that a QVariantMap is returned (rather than
QString) with various meta data, depending on the sync output. There is
a descriptive message for each sync_output record, but it comes along
with hard data, such as what property is related to the message, any
particulars about the database (e.g. name, its index within the targets
definition). Additionally the messages themselves have been modified,
with some former details that were included in the description pushed to
the various meta-data instead. A set of error IDs could be created as well.

The information can be used at the application level, or used by methods
within the plugin itself (e.g. console output, log file).

I'll push those changes since they over write the original error
handling that needed removal/modification.

Kevin

> The information can be used at the application level, or used by methods
> within the plugin itself (e.g. console output, log file).
>
> I'll push those changes since they over write the original error
> handling that needed removal/modification.

Nice stuff actually. Much cleaner than I expected! It should lend itself extremely nicely for test cases as well. I wouldn't have expected this in this MR but I'm not going to complain now ;-)

FAILED: Continuous integration, rev:118
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/4/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/4/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/4/rebuild

Den 09/08/2013 11:20, skrev Christian Dywan:
> These are the doc errors I'm currently seeing with the latest changes. They need to be resolve before the branch can be merged:
>
> ./src/database.cpp:427: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/database.cpp:428: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/database.cpp:429: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:33: warning: Cannot find Synchronizer specified with \class in any header file
> ./src/synchronizer.cpp:156: warning: Cannot find Synchronizer::source specified with \property in any header file
> ./src/synchronizer.cpp:173: warning: Cannot find Synchronizer::targets specified with \property in any header file
> ./src/synchronizer.cpp:189: warning: Cannot find Synchronizer::synchronize specified with \property in any header file
> ./src/synchronizer.cpp:202: warning: Cannot find Synchronizer::errors specified with \property in any header file
> ./src/synchronizer.cpp:214: warning: Cannot find Synchronizer::errors specified with \property in any header file
> ./src/synchronizer.cpp:358: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:359: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:360: warning: Unknown command '\return'
> ./src/synchronizer.cpp:783: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:817: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:818: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/synchronizer.cpp:889: warning: Unknown command '\param'
> [Maybe you meant '\part'?]
> ./src/database.cpp:398: warning: Undocumented parameter doc_id in Database::getCurrentDocRevisionNumber()
> ./src/database.cpp:255: warning: No documentation for 'Database::getDocumentContents()'
> ./src/database.cpp:324: warning: Undocumented parameter doc_id in Database::getNextDocRevisionNumber()
> ./src/database.cpp:818: warning: No documentation for 'Database::getSyncLogInfo()'
> ./src/database.cpp:792: warning: No documentation for 'Database::listTransactionsSince()'
> ./src/database.cpp:619: warning: No documentation for 'Database::resetModel()'
> ./src/database.cpp:463: warning: No documentation for 'Database::updateDocRevisionNumber()'
> ./src/database.cpp:425: warning: Undocumented parameter transaction_id in Database::updateSyncLog()
> ./src/database.cpp:425: warning: Undocumented parameter generation in Database::updateSyncLog()
> ./src/database.cpp:425: warning: Undocumented parameter insert in Database::updateSyncLog()
> ./src/database.cpp:425: warning: Undocumented parameter uid in Database::updateSyncLog()
> ./src/query.cpp:161: warning: No documentation for 'Query::resetModel()'

Hopefully all fixed now. Could not find anything specific to resolve the
warnings about \property, but maybe this is resolved by the change to
fix the warning about \class.

Also \param is used elsewhere in source files, but perhaps was not used
properly in these instances. The references have been remov...

Den 08/08/2013 16:24, skrev Christian Dywan:
> Review: Needs Fixing
>
> The code deals with id's being wrapped in {} in places. That's based on behavior of the core code; I think we need a newid() function and solve this in one place.
>
Have not done anything about this. I'm not sure it needs attending to
ahead of higher priority items.
> There's a few instances of the pattern "if (!query.exec()) qDebug" which should probably be using setError.

Have not added any new qDebug messages yet.
>
> resetModel can be used in more places currently still doing its equivalent calls.
Still unsure about this comment -- maybe it was mentioned in another thread.

>
> getValidTargets() and synchronizeTargets() hard-code HTML formatting - that's not good if we plan to see errors in console or XML and will make those cases harder to deal with. The messages should be predictable without using markup - if needed, it's still an option to use regex.
> m_errors.append("<b><font color=\"red\">Database "+index_number+"</font></b>: F

Discussed in another thread. Has been changed completely. No more markup
is included in output messages.

>
> FIXMEs that ought to be sorted out:
> ##KW## The two lists below are currently not used for anything
> ##KW## maybe this can be removed as it is replaced by revision_number
Already removed.
>
> Unused code:
> //targetDb->updateSyncLog(true, QString uid, QString generation, QString transaction_id);
Already removed?
> //double target_replica_generation = replyMap["target_replica_generation"].toDouble();
Already commented on this -- variable might be used in further
modifications. Commented out to avoid seeing warning during builds.

> This should probably say the real app name + " (U1Db-Qt)"
> request.setRawHeader("User-Agent", "My app name v0.1");
> request.setRawHeader("X-Custom-User-Agent", "My app name v0.1");
Already fixed.
>
> For good measure processDataFromRemoteServer might want to qDebug if an unexpected i.key() is seen?

Have not touched this -- will add an else clause.

>
> This should not be writable, it's only set internally
> Q_PROPERTY(QList<QString> errors READ getErrors WRITE setErrors NOTIFY errorsChanged)

Modified to sync_output, QList<QVariant> and read only (commit message
has more details).

>
> "where u1db has been downloaded/branched"
> This is very ambiguous and doesn't say where to obtain it. It should say
> "where the u1db Python reference implemented has been downloaded from lp:u1db"
Already fixed.

Kevin

Den 09/08/2013 12:50, skrev Christian Dywan:
>> The information can be used at the application level, or used by methods
>> within the plugin itself (e.g. console output, log file).
>>
>> I'll push those changes since they over write the original error
>> handling that needed removal/modification.
> Nice stuff actually. Much cleaner than I expected! It should lend itself extremely nicely for test cases as well. I wouldn't have expected this in this MR but I'm not going to complain now ;-)

It was actually quite easy to implement. Took less than 30 minutes to
put together.

FAILED: Continuous integration, rev:119
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/5/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/5/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/5/rebuild

>> There's a few instances of the pattern "if (!query.exec()) qDebug"
>> which should probably be using setError.
>
> Have not added any new qDebug messages yet.

Whoops. Misread this one. Added qDebug statements where there was
nothing. Will fix to use setError.

Kevin

FAILED: Continuous integration, rev:120
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/6/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/6/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/6/rebuild

Den 09/08/2013 13:48, skrev Kevin Wright:
>>> There's a few instances of the pattern "if (!query.exec()) qDebug"
>>> which should probably be using setError.
>> Have not added any new qDebug messages yet.
> Whoops. Misread this one. Added qDebug statements where there was
> nothing. Will fix to use setError.
>
> Kevin
>

Hopefully all done now.

Kevin

FAILED: Continuous integration, rev:122
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/7/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/7/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/7/rebuild

> Den 09/08/2013 11:20, skrev Christian Dywan:
> > These are the doc errors I'm currently seeing with the latest changes. They
> need to be resolve before the branch can be merged:
> Hopefully all fixed now. Could not find anything specific to resolve the
> warnings about \property, but maybe this is resolved by the change to
> fix the warning about \class.
>
> Also \param is used elsewhere in source files, but perhaps was not used
> properly in these instances. The references have been removed in the
> latest commit, but perhaps this needs to be looked at again.

There's still 17 qdoc warnings. See https://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/7/console I locally see the same.

FAILED: Continuous integration, rev:123
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/8/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/8/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/8/rebuild

FAILED: Continuous integration, rev:124
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/9/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/9/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/9/rebuild

I get a unit test failure with the latest update:

  <testcase result="fail" name="U1dbDatabase::test_1_databasePopulated">
    <failure message="Compared values are not the same
   Actual (): false
   Expected (): true" result="fail"/>
  </testcase>

Running "qmltestrunner -import ./_build/modules -input ./tests" gives me the line number:

tests/tst_database.qml(75)

Den 12/08/2013 11:26, skrev Christian Dywan:
> Review: Needs Fixing
>
> I get a unit test failure with the latest update:
>
> <testcase result="fail" name="U1dbDatabase::test_1_databasePopulated">
> <failure message="Compared values are not the same
> Actual (): false
> Expected (): true" result="fail"/>
> </testcase>
>
> Running "qmltestrunner -import ./_build/modules -input ./tests" gives me the line number:
>
> tests/tst_database.qml(75)

The test message seems ambiguous.

Compared values of what?

Kevin

There's also Jenkins brokenness with precise:
  Err http://ppa.launchpad.net precise/main i386 Packages
  404 Not Found

Not at all related to u1db-qt - I'm investigating what we can do there.

> Den 12/08/2013 11:26, skrev Christian Dywan:
> > Review: Needs Fixing
> >
> > I get a unit test failure with the latest update:
> >
> > <testcase result="fail" name="U1dbDatabase::test_1_databasePopulated">
> > <failure message="Compared values are not the same
> > Actual (): false
> > Expected (): true" result="fail"/>
> > </testcase>
> >
> > Running "qmltestrunner -import ./_build/modules -input ./tests" gives me the
> line number:
> >
> > tests/tst_database.qml(75)
>
> The test message seems ambiguous.
>
> Compared values of what?

Tell me about it, hte default messages of qmltestrunner suck :-(
But the line is:

compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]}) > -1, true)

So that translates to "Expected -1 but got something else from putDoc()"

Den 12/08/2013 11:35, skrev Christian Dywan:
>> Den 12/08/2013 11:26, skrev Christian Dywan:
>>> Review: Needs Fixing
>>>
>>> I get a unit test failure with the latest update:
>>>
>>> <testcase result="fail" name="U1dbDatabase::test_1_databasePopulated">
>>> <failure message="Compared values are not the same
>>> Actual (): false
>>> Expected (): true" result="fail"/>
>>> </testcase>
>>>
>>> Running "qmltestrunner -import ./_build/modules -input ./tests" gives me the
>> line number:
>>> tests/tst_database.qml(75)
>> The test message seems ambiguous.
>>
>> Compared values of what?
> Tell me about it, hte default messages of qmltestrunner suck :-(
> But the line is:
>
> compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]}) > -1, true)
>
> So that translates to "Expected -1 but got something else from putDoc()"
Yep, found it. putDoc returns a QString, but the test expects bool.

Not sure how to modify the test properly because the return value is
based on the UID of the database, which will be different with each new
database created -- in addition the function that retrieves the database
UID is private, so unless that is made public the only thing that could
be tested for is whether the return string is empty or not. Well, if the
return string contains ":1" that could also be tested for -- not quite a
good test but perhaps better than nothing.

Kevin

> > compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]}) > -1,
> true)
> >
> > So that translates to "Expected -1 but got something else from putDoc()"
> Yep, found it. putDoc returns a QString, but the test expects bool.
>
> Not sure how to modify the test properly because the return value is
> based on the UID of the database, which will be different with each new
> database created -- in addition the function that retrieves the database
> UID is private, so unless that is made public the only thing that could
> be tested for is whether the return string is empty or not. Well, if the
> return string contains ":1" that could also be tested for -- not quite a
> good test but perhaps better than nothing.

It sounds sensible to check the empty string - the test only checks if it succeeds or not at the moment. I would leave it to a separate task to extend it to see if that id is valid, works, etc.

Den 12/08/2013 11:55, skrev Christian Dywan:
>>> compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]}) > -1,
>> true)
>>> So that translates to "Expected -1 but got something else from putDoc()"
>> Yep, found it. putDoc returns a QString, but the test expects bool.
>>
>> Not sure how to modify the test properly because the return value is
>> based on the UID of the database, which will be different with each new
>> database created -- in addition the function that retrieves the database
>> UID is private, so unless that is made public the only thing that could
>> be tested for is whether the return string is empty or not. Well, if the
>> return string contains ":1" that could also be tested for -- not quite a
>> good test but perhaps better than nothing.
> It sounds sensible to check the empty string - the test only checks if it succeeds or not at the moment. I would leave it to a separate task to extend it to see if that id is valid, works, etc.
I misunderstood slightly what the original test was looking for...it was
checking for an int actually, and whether it was > -1. In any case I
have changed it to check the return string and whether != empty string.

The qmltestrunner program seems to hang sometimes. It is difficult to
know if it is running or not. Is that normal?

Kevin

Den 12/08/2013 12:02, skrev <email address hidden>:
> Den 12/08/2013 11:55, skrev Christian Dywan:
>>>> compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]}) >
>>>> -1,
>>> true)
>>>> So that translates to "Expected -1 but got something else from
>>>> putDoc()"
>>> Yep, found it. putDoc returns a QString, but the test expects bool.
>>>
>>> Not sure how to modify the test properly because the return value is
>>> based on the UID of the database, which will be different with each new
>>> database created -- in addition the function that retrieves the
>>> database
>>> UID is private, so unless that is made public the only thing that could
>>> be tested for is whether the return string is empty or not. Well, if
>>> the
>>> return string contains ":1" that could also be tested for -- not
>>> quite a
>>> good test but perhaps better than nothing.
>> It sounds sensible to check the empty string - the test only checks
>> if it succeeds or not at the moment. I would leave it to a separate
>> task to extend it to see if that id is valid, works, etc.
> I misunderstood slightly what the original test was looking for...it
> was checking for an int actually, and whether it was > -1. In any case
> I have changed it to check the return string and whether != empty string.
>
> The qmltestrunner program seems to hang sometimes. It is difficult to
> know if it is running or not. Is that normal?
>
> Kevin
>
>
Checking for an empty string does not appear to be working no matter
which way I try. Will keep at it.

However, it also appears that there is a check for the file name of the
database. The test is only the name of the file itself, but the value
recieved appears to be the absolute path.

Kevin

Den 12/08/2013 12:22, skrev <email address hidden>:
> Den 12/08/2013 12:02, skrev <email address hidden>:
>> Den 12/08/2013 11:55, skrev Christian Dywan:
>>>>> compare(myDatabase.putDoc({"animals": ["cat", "dog", "hamster"]})
>>>>> > -1,
>>>> true)
>>>>> So that translates to "Expected -1 but got something else from
>>>>> putDoc()"
>>>> Yep, found it. putDoc returns a QString, but the test expects bool.
>>>>
>>>> Not sure how to modify the test properly because the return value is
>>>> based on the UID of the database, which will be different with each
>>>> new
>>>> database created -- in addition the function that retrieves the
>>>> database
>>>> UID is private, so unless that is made public the only thing that
>>>> could
>>>> be tested for is whether the return string is empty or not. Well,
>>>> if the
>>>> return string contains ":1" that could also be tested for -- not
>>>> quite a
>>>> good test but perhaps better than nothing.
>>> It sounds sensible to check the empty string - the test only checks
>>> if it succeeds or not at the moment. I would leave it to a separate
>>> task to extend it to see if that id is valid, works, etc.
>> I misunderstood slightly what the original test was looking for...it
>> was checking for an int actually, and whether it was > -1. In any
>> case I have changed it to check the return string and whether !=
>> empty string.
>>
>> The qmltestrunner program seems to hang sometimes. It is difficult to
>> know if it is running or not. Is that normal?
>>
>> Kevin
>>
>>
> Checking for an empty string does not appear to be working no matter
> which way I try. Will keep at it.
>
> However, it also appears that there is a check for the file name of
> the database. The test is only the name of the file itself, but the
> value recieved appears to be the absolute path.

Scratch this last one. Seems to be a misunderstanding.

>
> Kevin

> There's also Jenkins brokenness with precise:
> Err http://ppa.launchpad.net precise/main i386 Packages
> 404 Not Found
>
> Not at all related to u1db-qt - I'm investigating what we can do there.

FYI A work-around for the issue is in place (real fix will take longer). The next automated build should run fine.

FAILED: Continuous integration, rev:125
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/10/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/10/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/10/rebuild

FAILED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/11/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/11/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/11/rebuild

ERROR: No artifacts found that match the file pattern "**/*test*.xml". Configuration error?
ERROR: '**/*test*.xml' doesn't match anything: '**' exists but not '**/*test*.xml'

I'm seeing to get this sorted. As far as I'm concerned, the build, tests and docs pass and this is down to Jenkins.

FAILED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/12/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/14/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/12/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/12/rebuild

FAILED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/13/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/13/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/13

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/13/rebuild

FAILED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/14/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/14/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/14/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/14/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/14

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/14/rebuild

PASSED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-armhf-ci/15

Click here to trigger a rebuild:
http://s-jenkins:8080/job/u1db-qt-ci/15/rebuild