Conversation
jbeder
left a comment
jbeder
left a comment
There was a problem hiding this comment.
I don't know what the issue might be with your application, but thanks for these tests. A few minor comments.
test/integration/load_node_test.cpp
Outdated
| } | ||
|
|
||
| TEST(LoadNodeTest, AliasAccessStream) { | ||
| std::ifstream input("integration/alias.yaml"); |
There was a problem hiding this comment.
No need to have this separate file-based loading; it causes various problems like worrying about working directories, etc.
Instead, if you're worried about flow vs. block, then you can keep the alias_access function and have two tests; otherwise, one test is fine.
There was a problem hiding this comment.
Feel free to to adapt and use only what you need.
While tracking down the issue, I tried variation along many directions. One was to load from an input...
There was a problem hiding this comment.
Do you mean you're abandoning this PR?
There was a problem hiding this comment.
If you don't think, it's useful, yes.
There was a problem hiding this comment.
I do think it's useful, and I added some comments before approval.
test/integration/load_node_test.cpp
Outdated
| EXPECT_TRUE(b.IsScalar()); | ||
|
|
||
| // a and b should be identical | ||
| EXPECT_STREQ(a.as<std::string>().c_str(), b.as<std::string>().c_str()); |
There was a problem hiding this comment.
You can just expect that they're equal; you don't need to convert to c_str.
test/integration/load_node_test.cpp
Outdated
| EXPECT_EQ(3, i); | ||
| } | ||
|
|
||
| static void alias_access(Node&& doc) { |
There was a problem hiding this comment.
If you do keep this function (see below):
- change its name to camel case
- put it in an anonymous namespace instead of being static
- make it take a const ref instead of an rvalue reference
- call it something that indicates it does some assertions, like VerifyAliasAccess
rhaschke
force-pushed
the
issue-alias-access
branch
from
5a4fbb8 to
67bd615
Compare
jbeder
left a comment
jbeder
left a comment
There was a problem hiding this comment.
Sorry for the delay. A couple minor comments, and I think this looks good.
|
|
||
| add_test(yaml-test ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/run-tests) | ||
| add_test(NAME yaml-test COMMAND run-tests | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) |
There was a problem hiding this comment.
This shouldn't be necessary since you're not using files, right?
| ASSERT_TRUE(B); | ||
| ASSERT_TRUE(B.IsMap()); | ||
|
|
||
| // A and B have the same content |
There was a problem hiding this comment.
I think A and B are actually the same node, so you can just check
ASSERT_TRUE(A.is(B));
or
ASSERT_TRUE(A == B);
(which is the same thing).
2 participants
I'm having trouble with accessing values in a yaml file like this:
For some reason, in my application, accessing values
B[key]fails, whileA[key]succeeds. For this reason, I thought there might be an issue in yaml-cpp and implemented appropriate unittests with a minimal example. However, the unittests work as expected. Now idea, how to proceed now...Nevertheless, the unittests might be interesting to you anyway. If haven't found particular tests for aliases/references.