Add unit test for expr_initializer.cpp #7795
Conversation
esteffin
force-pushed
the
esteffin/expr-initializer-unit-test
branch
2 times, most recently
from
a49341a
to
87786b4
Compare
esteffin
force-pushed
the
esteffin/expr-initializer-unit-test
branch
from
87786b4
to
d8a5fa6
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #7795 +/- ##
===========================================
- Coverage 78.59% 78.52% -0.08%
===========================================
Files 1696 1697 +1
Lines 193450 193627 +177
===========================================
- Hits 152049 152048 -1
- Misses 41401 41579 +178
☔ View full report in Codecov by Sentry. |
unit/util/expr_initializer.cpp
Outdated
| /// them the symbol_table in the environment. | ||
| static symbolt create_tag_populate_env( | ||
| const typet &type, | ||
| expr_initializer_test_environmentt &env) |
There was a problem hiding this comment.
⛏️ Given that this function only updates the symbol table, it might be better to pass a reference to the symbol table, rather then the whole test environment.
unit/util/expr_initializer.cpp
Outdated
| const typet &type, | ||
| expr_initializer_test_environmentt &env) | ||
| { | ||
| const type_symbolt symbol{"my_type_symbol", type, ID_C}; |
There was a problem hiding this comment.
⛏️ If this function were to be called multiple times in the same test, then it would be inserting duplicate symbol names into the symbol table. This seems like a potential trap when maintaining these tests in future. Consider using the existing get_fresh_aux_symbol from src/util/fresh_symbol.h in order to mitigate this risk.
There was a problem hiding this comment.
No.
The get_fresh_aux_symbol function will ALWAYS return a "normal" symbolt, and not a type_symbolt, so then the lookup in the namespace will fail as that requires the resolved symbolt to be a type_symbol.
| {"foo", signedbv_typet{32}}, {"bar", unsignedbv_typet{16}}}; | ||
| const struct_typet inner_struct_type{inner_struct_components}; | ||
| const struct_union_typet::componentst struct_components{ | ||
| {"fizz", bool_typet{}}, {"bar", inner_struct_type}}; |
There was a problem hiding this comment.
💡 Instead of using increasingly complex type construction code in unit tests, it might be worth considering writing the types in C and using unit/testing-utils/get_goto_model_from_c.h to build a symbol table from C source.
unit/util/expr_initializer.cpp
Outdated
| if(can_cast_type<c_enum_typet>(type)) | ||
| { | ||
| const c_enum_tag_typet c_enum_tag_type{symbol.name}; | ||
| return symbolt{"my_type_tag_symbol", c_enum_tag_type, ID_C}; |
There was a problem hiding this comment.
⛏️ I may have made similar comments before - however given that this symbol does not define a type, but rather has a type which is a tag variant, then featuring "type_tag" in the name seems misleading to me.
There was a problem hiding this comment.
Removed as the function is now just returning the tag_typet.
unit/util/expr_initializer.cpp
Outdated
| } | ||
|
|
||
| TEST_CASE( | ||
| "nondet_initializer simple array_type", |
There was a problem hiding this comment.
⛏️ It might be nice to specify the type instead of calling it simple - "nondet_initializer on fixed-size array of signed 8 bit bytes."
unit/util/expr_initializer.cpp
Outdated
| } | ||
|
|
||
| TEST_CASE( | ||
| "nondet_initializer double array_type", |
There was a problem hiding this comment.
⛏️ "double" here could be confused with the floating point double type on first glance. I suggest using the wording "array of arrays".
| const std::size_t elem_count = 3; | ||
| typet inner_array_type = | ||
| array_typet{inner_type, from_integer(elem_count, signedbv_typet{8})}; | ||
| typet array_type = |
There was a problem hiding this comment.
⛏️ If an array type has an "inner_array_type", does that make it an "outer_array_type"?
There was a problem hiding this comment.
Changed inner_array to sub_array_type, so now there is no "outer_array".
| const auto inner_expected = array_exprt{ | ||
| inner_array_values, | ||
| array_typet{ | ||
| signedbv_typet{8}, from_integer(elem_count, signedbv_typet{8})}}; |
There was a problem hiding this comment.
⛏️ This could just be the inner_array_type rather than duplicating the type construction right?
There was a problem hiding this comment.
Here I prefer to recreate the type explicitly as it guarantee more control.
unit/util/expr_initializer.cpp
Outdated
| side_effect_expr_nondett{unsignedbv_typet{16}, test.loc}}; | ||
| const struct_exprt expected_inner_struct_expr{ | ||
| expected_inner_struct_operands, inner_struct_type}; | ||
| const exprt::operandst expected_struct_operands{ |
There was a problem hiding this comment.
⛏️ Wouldn't the "operands" of a struct just be the "fields"?
unit/util/expr_initializer.cpp
Outdated
| side_effect_expr_nondett{signedbv_typet{8}, test.loc}}}}; | ||
| const union_typet union_type{union_components}; | ||
| const auto result = nondet_initializer(union_type, test.loc, test.ns); | ||
| REQUIRE(!result.has_value()); |
There was a problem hiding this comment.
⛏️ Prefer the use of REQUIRE_FALSE( over REQUIRE(!
esteffin
force-pushed
the
esteffin/expr-initializer-unit-test
branch
from
d8a5fa6
to
5646669
Compare
expr_initializer.cppwas not unit-tested.Before extending
expr_initializerfunctionalities (in #7392) this PR adds unit-tests for thenondet_initializerfunction.