On 11 August 2016 at 00:41, Zachary Turner via lldb-commits
<lldb-commits@lists.llvm.org> wrote:
> Couple of random style comments below. You don't have to fix these, just
> something to think about in the future.
>
>
>
> On Wed, Aug 10, 2016 at 4:33 PM Greg Clayton via lldb-commits
> <lldb-commits@lists.llvm.org> wrote:
>>
>> +
>> +TEST_F(PythonDataObjectsTest, TestExtractingUInt64ThroughStructuredData)
>> +{
>> + // Make up a custom dictionary with "sys" pointing to the `sys`
>> module.
>> + const char *key_name = "addr";
>> + const uint64_t value = 0xf000000000000000ull;
>> + PythonDictionary python_dict(PyInitialValue::Empty);
>> + PythonInteger python_ull_value(PyRefType::Owned,
>> PyLong_FromUnsignedLongLong(value));
>> + python_dict.SetItemForKey(PythonString(key_name), python_ull_value);
>> + StructuredData::ObjectSP structured_data_sp =
>> python_dict.CreateStructuredObject();
>> + EXPECT_TRUE((bool)structured_data_sp);
>
> EXPECT_NE(nullptr, structured_data_sp);
>
> would be preferable here. If EXPECT_TRUE fails, you will just an error
> message saying it was false when it should have been true. If EXPECT_NE
> fails, you will get an error message telling you the expected value and the
> actual value. This is most useful when the actual and expected values are
> integers or strings, but the same concept applies anywhere. Using the
> proper EXPECT macro gives you a better error message. Same goes in a few
> other places below.
>
>>
>> + if (structured_data_sp)
>> + {
>> + StructuredData::Dictionary *structured_dict_ptr =
>> structured_data_sp->GetAsDictionary();
>> + EXPECT_TRUE(structured_dict_ptr != nullptr);
>> + if (structured_dict_ptr)
>> + {
>> + StructuredData::ObjectSP structured_addr_value_sp =
>> structured_dict_ptr->GetValueForKey(key_name);
>> + EXPECT_TRUE((bool)structured_addr_value_sp);
>> + const uint64_t extracted_value =
>> structured_addr_value_sp->GetIntegerValue(123);
>> + EXPECT_TRUE(extracted_value == value);
>
> Here's an example of where EXPECT_EQ(value, extracted_value) would be really
> helpful. Without it, you'd need to debug the test to to see what the actual
> value is. With it, it will print both values so you can often easily
> determine the cause of the failure without any additional effort.
>
>>
>> + }
>> + }
>> +}
>
> As a general rule of thumb, we should probably avoid conditionals in unit
> tests unless they're really necessary. It's easy to end up in cases where
> your test ends up not testing something because it's behind a conditional.
> EXPECT_NE(nullptr, foo_sp) is probably fine, then just assume it's non null.
+1
Also note the presence of the ASSERT_XXX macros. The work the same as
the EXPECT_ versions, but also abort the current test (more precisely,
they just issue a "return", as gtest does not rely on exceptions).
That way you can do ASSERT_NE(nullptr, foo_sp), and in the rest of the
function you can assume that the check was successful.
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
