Merge lp:~marcustomlinson/unity-scopes-api/lp-1410125 into lp:unity-scopes-api

Proposed by Marcus Tomlinson
Status: Merged
Approved by: Michi Henning
Approved revision: 277
Merged at revision: 278
Proposed branch: lp:~marcustomlinson/unity-scopes-api/lp-1410125
Merge into: lp:unity-scopes-api
Diff against target: 53 lines (+12/-3)
3 files modified
src/scopes/internal/JsonCppNode.cpp (+5/-0)
src/scopes/internal/JsonSettingsSchema.cpp (+5/-1)
test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp (+2/-2)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/lp-1410125
Reviewer Review Type Date Requested Status
Michi Henning (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+246264@code.launchpad.net

Commit message

Check if root_ pointer is valid before executing JsonCppNode::has_node() logic

To post a comment you must log in.
Revision history for this message
Michi Henning (michihenning) wrote :

This looks reasonable. But why haven't we seen this before now?

review: Needs Information
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
277. By Marcus Tomlinson

Fixed JsonSettingsSchema_test

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

> This looks reasonable. But why haven't we seen this before now?

In particular, the test (as it was originally) passes when I run this locally on my Vivid machine, but fails on Jenkins. Why?

review: Needs Information
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> > This looks reasonable. But why haven't we seen this before now?
>
> In particular, the test (as it was originally) passes when I run this locally
> on my Vivid machine, but fails on Jenkins. Why?

Maybe a new version of JsonCpp? My version is "0.6.0~rc2-3.1ubuntu1".

Here's the output when I run the test on my Vivid machine:

[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from JsonSettingsSchema
[ RUN ] JsonSettingsSchema.basic
unknown file: Failure
C++ exception with description "unity::ResourceException: JsonSettingsSchema(): expected value of type object for "parameters", id = "string_no_default2"" thrown in the test body.
[ FAILED ] JsonSettingsSchema.basic (1 ms)
[ RUN ] JsonSettingsSchema.exceptions
/home/marcustomlinson/Projects/work/unity-scopes-api/lp-1410125/test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp:461: Failure
Value of: e.what()
  Actual: "unity::ResourceException: JsonSettingsSchema(): expected value of type object for \"parameters\", id = \"x\""
Expected: "unity::ResourceException: JsonSettingsSchema(): missing \"parameters\" definition, id = \"x\""
[ FAILED ] JsonSettingsSchema.exceptions (1 ms)
[ RUN ] JsonSettingsSchema.empty_then_with_location
[ OK ] JsonSettingsSchema.empty_then_with_location (1 ms)
[----------] 3 tests from JsonSettingsSchema (3 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (4 ms total)
[ PASSED ] 1 test.
[ FAILED ] 2 tests, listed below:
[ FAILED ] JsonSettingsSchema.basic
[ FAILED ] JsonSettingsSchema.exceptions

 2 FAILED TESTS

Revision history for this message
Michi Henning (michihenning) wrote :

> Maybe a new version of JsonCpp? My version is "0.6.0~rc2-3.1ubuntu1".

Same here, and that's what's also on Jenkins. Weird.

Maybe the best fix would be to not test the full exception string, seeing that the text of the exception comes from the underlying library. One option would be to just expect a ResourceException for the failing test.

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

>
> > Maybe a new version of JsonCpp? My version is "0.6.0~rc2-3.1ubuntu1".
>
> Same here, and that's what's also on Jenkins. Weird.
>
> Maybe the best fix would be to not test the full exception string, seeing that
> the text of the exception comes from the underlying library. One option would
> be to just expect a ResourceException for the failing test.

That exception is ours, not from the underlying library. If you can get the test to fail like it does on my machine, you too will be confused as to why it passed before :/

I recently did a update + dist-upgrade. It really feels like a dependancy has changed, and the only one I can think of is libjsoncpp.

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Looking at the first few lines of Setting::get_enum_default(), it seems that a Json::Value object could be an "Object" and a "Null" at the same time. Now it seems that this has changes to be one or the other.

Revision history for this message
Michi Henning (michihenning) wrote :

OK. After updating and using your changes, it works locally and on Jenkins, as far as I can tell. Let's merge this. If we stumble across this again, we can do something more fancy.

review: Approve
Revision history for this message
Michi Henning (michihenning) wrote :

Pawel, could please merge this before anything else? Everyone is falling over the failing test at the moment. (The failure is bogus, caused by a change in json-cpp.)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/scopes/internal/JsonCppNode.cpp'
2--- src/scopes/internal/JsonCppNode.cpp 2014-08-18 08:12:22 +0000
3+++ src/scopes/internal/JsonCppNode.cpp 2015-01-13 11:45:59 +0000
4@@ -252,6 +252,11 @@
5
6 bool JsonCppNode::has_node(std::string const& node_name) const
7 {
8+ if (!root_)
9+ {
10+ throw unity::LogicException("Current node is empty");
11+ }
12+
13 return root_.isMember(node_name);
14 }
15
16
17=== modified file 'src/scopes/internal/JsonSettingsSchema.cpp'
18--- src/scopes/internal/JsonSettingsSchema.cpp 2014-11-20 21:33:33 +0000
19+++ src/scopes/internal/JsonSettingsSchema.cpp 2015-01-13 11:45:59 +0000
20@@ -181,7 +181,11 @@
21 void Setting::set_default_value(Json::Value const& v, Type expected_type)
22 {
23 auto v_param = v[parameters_key];
24- if (!v_param.isObject())
25+ if (v_param.isNull())
26+ {
27+ return;
28+ }
29+ else if (!v_param.isObject())
30 {
31 throw ResourceException("JsonSettingsSchema(): expected value of type object for \"parameters\", id = \"" + id_ + "\"");
32 }
33
34=== modified file 'test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp'
35--- test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp 2014-11-20 21:33:33 +0000
36+++ test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp 2015-01-13 11:45:59 +0000
37@@ -397,7 +397,7 @@
38 {
39 "id": "x",
40 "type": "number",
41- "displayName": "X",
42+ "displayName": "X",
43 "parameters": {
44 "defaultValue": "banana"
45 }
46@@ -457,7 +457,7 @@
47 }
48 catch (ResourceException const& e)
49 {
50- EXPECT_STREQ("unity::ResourceException: JsonSettingsSchema(): missing \"parameters\" definition, id = \"x\"",
51+ EXPECT_STREQ("unity::ResourceException: JsonSettingsSchema(): missing \"displayName\" definition, id = \"x\"",
52 e.what());
53 }
54

Subscribers

People subscribed via source and target branches

to all changes: