Unit test cleanup #1534
Unit test cleanup #1534
Conversation
0760f05 to
a891bf3
Compare
|
@thk123 @mgudemann @chrisr-diffblue @NlightNFotis I guess this concerns all of us so please take a look. |
| @@ -1,4 +1,4 @@ | |||
| class RecursiveGeneric extends SimpleRecursiveGeneric<RecursiveGeneric> | |||
| class RecursiveGeneric<T extends RecursiveGeneric<T>> | |||
There was a problem hiding this comment.
This was supposed to be just renaming, I will correct this.
There was a problem hiding this comment.
This is actually correct, the original file RecursiveGeneric got renamed to RecursiveGeneric_Derived, and the original SimpleRecursiveGeneric got in turn renamed to RecursiveGeneric
| @@ -1,6 +1,4 @@ | |||
| class GenericArray<T> | |||
| { | |||
| public T [] t; | |||
There was a problem hiding this comment.
Is it also worth adding a test for something like a field of type SomeClass<T>[] ?
| public static void processUpperBoundInterfaceGeneric(SimpleGeneric<? extends BasicInterface> x) { | ||
| assert(x.t.getX() == 4); | ||
| public static void processUpperBoundInterfaceGeneric(Generic<? extends | ||
| Interface> x) |
There was a problem hiding this comment.
Looks like your editor has done some auto-formatting here - could you make the line wrapping more readable? Maybe break after the ( and then indent?
| public static void processUpperBoundClassGeneric(SimpleGeneric<? extends Foo> x) { | ||
| assert(x.t.getX() == 4); | ||
| public static void processUpperBoundClassGeneric(Generic<? extends | ||
| Interface_Implementation> x) |
There was a problem hiding this comment.
Ditto with formatting here.
| public static void processLowerBoundGeneric(SimpleGeneric<? super Foo> x, Foo assign) { | ||
| x.t = assign; | ||
| public static void processLowerBoundGeneric(Generic<? super | ||
| Interface_Implementation> x, Interface_Implementation assign) |
There was a problem hiding this comment.
And same again here with the formatting
| } | ||
|
|
||
| // It is not legal Java to specify both an upper and lower bound | ||
| // public static void processBoundSuperClassGeneric(SimpleGeneric<? extends Object super Foo> x, Foo assign) { | ||
| // x.t = assign; | ||
| // public static void processBoundSuperClassGeneric(Generic<? extends Object super Interface_Implementation> x, Interface_Implementation assign) |
There was a problem hiding this comment.
These commented out parts are presumably not expected to be supported yet? While we may not have unit tests to support these features yet, is it worth still having this code in the .java/.class files anyway to at least test/prove that we at least don't crash when encountering them? Alternatively, if we do currently crash when encountering them, is there a Jira issue covering that crash, and if so, it would be nice if that Jira issue was reference from within these comments, e.g. // This currently causes a crash, but should be re-enabled once TG-1234 is fixed - having comments like that helps make it easier to re-enable useful tests once things get fixed, or to at least understand why something is commented out :-)
There was a problem hiding this comment.
See comment above - this isn't actually valid Java (it isn't clear to me what has even changed on this line?)
There was a problem hiding this comment.
As Thomas pointed out, this is not valid Java code but it may be a good idea to leave it here, in case someone would want to extend the test and would ponder about the same example ;) The only thing that changed here really is the renaming and removing the assignment.
|
|
||
| SCENARIO( | ||
| "java_bytecode_parse_bounded_generic_inner_classes", | ||
| "[core][java_bytecode][java_bytecode_parse_generics]") |
There was a problem hiding this comment.
Given that inner classes seem to throw up a few bugs in several parts of test-generator, is it worth us starting to add an additional tag to unit tests involving inner classes? e.g. [java_bytecode_inner_classes] ?
There was a problem hiding this comment.
Good point. Shouldn't it be [java_bytecode_parse_generics_inner_classes] though? Or would you make it for general inner classes too? Also, not all tests that have some inner class are focusing specifically on testing aspects of inner classes, some just use it for convenience. Should all of them be tagged then?
| } | ||
| } | ||
|
|
||
| THEN("The generic fields should be annotated with concrete types") |
There was a problem hiding this comment.
Should this THEN block be inside a WHEN block?
| REQUIRE_FALSE(is_java_generics_class_type(java_class_type)); | ||
|
|
||
| // TODO (tkiley): Extend this unit test when bounds are correctly | ||
| // parsed. |
There was a problem hiding this comment.
Jira issue covering that? ;-)
| auto generic_param_iterator = | ||
| java_generics_class_type.generic_types().cbegin(); | ||
|
|
||
| // The first parameter should be called K |
There was a problem hiding this comment.
There seems to be a couple of places where we do things like 'first type parameter should be called K' - could the code for that be factored out into a function in test_utils/require_type or somewhere similar?
|
|
||
| REQUIRE(java_generics_class_type.generic_types().size() == 2); | ||
|
|
||
| auto generic_param_iterator = |
There was a problem hiding this comment.
What is the reason for using an iterator here? Since you are explicitly accessing the two elements of the vector, one after the other and manually bumping the iterator (rather than doing an actual loop using the iterator), can't you just directly index the elements? e.g. java_generics_class_type.generic_types().at(0) and java_generics_class_type.generic_types().at(1) ?
| REQUIRE( | ||
| java_generics_class_type_var(1, java_generics_class_type) == | ||
| twoelementinner_name + "::V"); | ||
| } |
There was a problem hiding this comment.
Since the code for the first and second parameters is duplicate apart from the identifier name, it's worth thinking about whether this can be factored our into a helper function - probably the same one I previously mentioned :-)
| const symbolt &derived_symbol=new_symbol_table.lookup_ref(class_prefix); | ||
| const class_typet &derived_class_type= | ||
| const symbolt &derived_symbol = new_symbol_table.lookup_ref(class_prefix); | ||
| const class_typet &derived_class_type = | ||
| require_symbol::require_complete_class(derived_symbol); | ||
|
|
||
| // TODO(tkiley): Currently we do not support extracting information |
There was a problem hiding this comment.
Jira reference would be nice...
There was a problem hiding this comment.
I agree - I can't find a relevant ticket, if you could create one @svorenova that would be appreciated.
|
|
||
| const symbolt func_symbol = | ||
| new_symbol_table.lookup_ref(process_func_name); | ||
| const code_typet func_code = to_code_type(func_symbol.type); |
There was a problem hiding this comment.
Maybe use require_code here instead of to_code_type ? Or possibly even add a require_code_symbol to testing_utils/require_symbol.cpp ?
| REQUIRE(is_java_generic_type(param_x.type())); | ||
|
|
||
| const java_generic_typet generic_x = | ||
| to_java_generic_type(param_x.type()); |
There was a problem hiding this comment.
For this pattern of is_java_generic_type, followed by to_java_generic_type, it's probably worth factoring out into a require_java_generic_type helper function - https://github.com/diffblue/cbmc/pull/1460/files#diff-6dc35e009e58da05abb1a1765acdd9f7R77 might have something you can steal.
There was a problem hiding this comment.
These look useful Chris - would you mind cherry picking that commit @svorenova (or alternatively you can pull it into a PR @chrisr-diffblue and then base this on top of it).
| const symbol_typet &subtype=to_symbol_type(t_component.subtype()); | ||
|
|
||
| const pointer_typet &t_component = to_pointer_type( | ||
| type.get_component("t").type()); |
There was a problem hiding this comment.
Can you use the require_pointer helper function here?
|
|
||
| SCENARIO( | ||
| "java_bytecode_parse_generics", | ||
| "java_bytecode_parse_generic_class_one_param", |
There was a problem hiding this comment.
I wonder whether if we have changed the name of the scenario, whether we should also change the filename? I know it's pretty easy to do an all-project search for the scenario name, but it might make sense if all the unit test files are named to match their scenario name?
There was a problem hiding this comment.
As you mention below, there are two scenarios in the file, the names of the scenarios are prefixed with the name of the file. I discussed with @thk123 and created a task in JIRA (TG-1287) to re-organise the whole folder with unit tests for generics and that would decide the convention. Do you agree with keeping it as it is for now and resolving it in the mentioned task?
| @@ -16,307 +18,148 @@ | |||
| #include <util/language.h> | |||
| #include <util/message.h> | |||
| #include <java_bytecode/java_bytecode_language.h> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
Is this left over debug?
| symbol_typet &bound_type=to_symbol_type(sub_type); | ||
| REQUIRE(bound_type.get_identifier()=="java::java.lang.Object"); | ||
| } | ||
| const java_generic_parametert &generic_param = generic_types[0]; |
There was a problem hiding this comment.
I might be tempted here to drop the generic_types variable and just use the compound expression entirely - e.g. const java_generic_parametert &generic_param = java_generics_class_type.generic_types()[0]
| new_symbol_table | ||
| .has_symbol("java::generics$bound_element.f:()Ljava/lang/Number;")); | ||
| SCENARIO( | ||
| "java_bytecode_parse_generic_class_two_param", |
There was a problem hiding this comment.
Ah, I see you've split the scenario into multiple scenarios. I guess there is a design question to ask around whether we want one scenario per file or not. I'm open either way, but worth considering.
|
|
||
| require_type::require_pointer( | ||
| field_component.type(), symbol_typet | ||
| (class_prefix + "$InnerClass")); |
There was a problem hiding this comment.
Nit: Break after the ( - it's less likely to trick someone into thinking you are passing three parameters to require_pointer :-)
There was a problem hiding this comment.
Did clang-format produce this?
| const java_generic_parametert &generic_param = generic_variables[0]; | ||
| REQUIRE( | ||
| generic_param.type_variable() == | ||
| symbol_typet(class_prefix + "::T")); |
There was a problem hiding this comment.
Nit: I'm not sure if this is the new clang-format style or not (apologies for the noise if it is) - but indenting this line a bit would make it easier to see that it is the RHS of the == operator. If this is the clang-format style, then just ignore me :-)
| java_generic_inst_parametert( | ||
| symbol_typet( | ||
| "java::Interface_Implementation"))); | ||
| } |
There was a problem hiding this comment.
Consider factoring the above two blocks into a helper function that you can call on generic_variables[0] and generic_variables[1] ?
| symbol_typet(class_prefix + "::T")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ditto with these two code blocks, which are even more identical to each other.
There was a problem hiding this comment.
Apologies I originally wrote this 😊 but I agree this would be better
|
|
||
| #include <java_bytecode/java_bytecode_language.h> | ||
|
|
||
| #include <iostream> |
| generic_param.type_variable() == | ||
| symbol_typet(class_prefix + "::T")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like the above two blocks could be factored out too.
|
|
||
| THEN("The bounds are set correctly") | ||
| { | ||
| // TODO: the bounds are not parsed yet; extend tests when fixed |
|
Thank you @chrisr-diffblue for a detailed review, all comments are taken care of now (although some of them still show up above, I assume that's Github's inability to sense the change in formatting). And thank you for the awesome functions, they made everything so much easier! @thk123 I also addressed all your comments and added a test for generic fields. |
dd1d23d to
92b1e56
Compare
| new_symbol_table.lookup_ref(process_func_name); | ||
| const code_typet func_code = | ||
| require_type::require_code(func_symbol.type, 0); | ||
| // TODO: should point to something else than Object?? - possibly |
There was a problem hiding this comment.
Since T doesn't have a bound for returnSimpleField I think there is no TODO needed here since it is already correctly marked as generic
There was a problem hiding this comment.
So the Object here acts as a bound (i.e., no bound)?
|
Will merge once CI passes - prod me if I forget 👍 |
A unit test for inner classes and a unit test for a basic generic class
There is a new one in require_type with the same functionality
92b1e56 to
51cbfc9
Compare
e8b3cb9 Merge remote-tracking branch 'upstream/develop' into smowton/merge/develop_20171116 dc4a293 Merge pull request diffblue#1594 from reuk/reuk/cmake-fixup 48fc3d4 Merge pull request diffblue#1592 from antlechner/antonia/char-escape 538eef6 Merge pull request diffblue#1577 from smowton/smowton/fix/dependence_graph_inconsistency d3d632d Use multi-argument form of FILE command 81e56cc Tidy up CMakeLists f7141c0 Merge pull request diffblue#1582 from romainbrenguier/refactor/numerical-cast 8ed1023 Use UTF-16 conversion function in expr2java a53f5bf Split UTF-16 conversion code into two cases e0ad069 Merge pull request diffblue#1558 from NathanJPhillips/feature/complete-journalling_symbol_table 69d1a52 Added usages of base class symbol table 3e42a8d Add comment on has_symbol a2b45e3 Update to journalling symbol table 7aa80ad Remove lookup_impl - it won't work for recording symbol table and adds complexity cdbac8c Sort output of symbol_tablet::show 2ef1c94 Fix bug where move from const symbol collections 8035397 Style improvements 6dae8e8 Merge pull request diffblue#1515 from smowton/smowton/admin/codeowners 5297646 another ranged for 3d66779 Merge branch 'develop' of github.com:diffblue/cbmc into develop 4b5467c another ranged for f5dbfd4 Merge pull request diffblue#1589 from reuk/reuk/fewer-exceptions 8e99272 use ranged for 95cf5c3 Add directories without code owners and adapt code owners 8da6a81 Replace try-catch with nullptr checks 9ff48e0 Add numeric_cast template for numeric conversion af31813 Merge pull request diffblue#1575 from svorenova/nested_generics_tg1301 21b4e7e Extend unit tests to test for nested generics cf47dcb Extending parsing of generics to parse nested generic types 1aefb09 Merge pull request diffblue#1547 from smowton/smowton/feature/remove_virtual_functions_single_call 2b4ed77 Merge pull request diffblue#1579 from smowton/smowton/fix/cmdline_destructor 7305506 Merge pull request diffblue#1580 from smowton/smowton/fix/cast_materialised_temporary 87b9de1 Remove pointless typecasts a59dea6 Add unit test checking dependence graph consistency 80e66ba Remove virtual functions: expose single-call entry-point ffe02e4 Remove useless cmdlinet::clear() call ae34e9b Merge pull request diffblue#1578 from thk123/bugfix/specalised-classes 779d0aa Merge pull request diffblue#1574 from diffblue/taint-memcpy-develop 28a4846 Merge pull request diffblue#1568 from smowton/smowton/fix/java_div_by_zero ffd089f Constructed class to mimic the original class in all but name of symbol 7f53f02 Merge pull request diffblue#1569 from thk123/bugfix/TG-1403/generic-field-arrays 1abc75e Dependence graph: ensure grapht representation is consistent with domain e03b0cb Abstract interpreter: add finalize hook fa7d62a Makefile for goto-analyzer-taint-ansi-c 758ebb3 transfer taint on memcpy and memmove d0a844b Assert denominator non-zero when Java runtime exceptions are disabled e5744b2 Reorder code owner definition according to change risk 0f98cb4 Removed redundant if statement ffa104c Enforce condition that generic references must refer to generic classes 6e06fbd Extending tests to deal with specialising with arrays when array fields a01a0f2 Extend the specialisation code to handle generic fields 1ccbf83 Correctly handle generic classes that have a array field f60d8c8 Unit utility for symbol types 21a33fa Renaming to_java_generic_class_type to remove spurious s 94ffce3 Merge pull request diffblue#1567 from mgudemann/mgudemann/feature/support_arrays_in_generic_parameters 5be97db Create new and adapt existing unit tests for generic array param ef6b4af Post-fix arrays as generic types with their element type 4db6fc6 Merge pull request diffblue#1553 from mgudemann/bugfix/initialize_pointer_width_in_unit_test b17ed58 Merge pull request diffblue#1555 from thk123/feature/remove-redundant-specalisation-code 9b34cdb Merge pull request diffblue#1564 from owen-jones-diffblue/bugfix/object-numbering-references 52d4326 Merge pull request diffblue#731 from tautschnig/more-rewriting 51133db Remove test checking don't specalise unspecalised generic types bf10b1b Manually call specalisation code bba9f76 Remove redundant regression test 3047678 Removed old method of specalising generics 2db8c45 Merge pull request diffblue#982 from tautschnig/pointer-handling fb532e8 Generalize ID_malloc to ID_allocate with optional zero-init 3c47ccb Use invariant annotations instead of asserts ebd5343 More unwinding should not yield additional assertion failures cc659c9 Use a known constant offset when dereferencing c507ccf Update all constant offsets, not just 0 0361c2a Merge pull request diffblue#1534 from svorenova/unit-test-cleanup f653f85 Merge pull request diffblue#263 from diffblue/owen/fix-memory-bug ede0e8c Fix bug that can cause segfault 51cbfc9 Deleting a utility function for generics 03438bb Disabling part of unit test due to a bug e3019f2 Extending test for derived generics f5ec45a Adding JIRA tickets cont. 1fa8e2f Adding unit test for generic fields 398c88a Applying new utility functions for generics cce7814 Refactoring unit test utility functions to make them easier to use c1e1ba2 Applying new function for accessing elements of arrays e908f0c Updating utility functions to check generic/non-generic java classes d9d9ea1 Cleaning includes, unifying scenario names, adding JIRA references 2883bb1 Extending test for generic arrays de97e23 Adding unit test for nested generics c9a3716 Adding unit test for functions with generics 9db9947 Extending test for generic class 89b99ce Extending test for generic functions 3e6cf35 Extending test for signature/descriptor mismatch 80be2fd Extending and cleaning test for generic class with generic inner classes 2e2e34b Renaming unit test for generic inner classes to bounded generic inner classes c5b06e6 Breaking the old parse_generic_class into two unit tests d3ff11c Adding a utility for checking java generic class 707ebf6 Cleaning existing unit tests af3efea Renaming java files 14c00dc Simplify all expressions generated by flatten_byte_operators 71e9642 Extensions to simplify_byte_extract 81943f2 Split ID_and/ID_or vs ID_xor simplification 77236cc Avoid nesting of ID_with/byte_update by rewriting byte_extract to use the root object ddd3d03 Extended simplify for byte_update, typing 7064483 simplify_typecast: simplify more pointer arithmetic 2b18e0c Merge pull request diffblue#1562 from NathanJPhillips/feature/extend-main_function_result 599a2f9 Merge pull request diffblue#264 from diffblue/smowton/fix/slice24_include de905e7 slice24 test: switch from malloc.h to stdlib.h 89a1132 Merge pull request diffblue#1559 from NathanJPhillips/bugfix/variable-scope 0aeb459 Tidied up get_main_symbol af2d3dd Merge pull request diffblue#1560 from NathanJPhillips/bugfix/catch-by-const-ref c8efb6f Fix bug that can cause segfault b7cc0ae Merge pull request diffblue#1561 from NathanJPhillips/bugfix/erroneous-replacement 7d66469 Typo in reachable 7de4858 Added copyright notice to fix linting error 476270b catch by const ref instead of by value or non-const ref 2f32aee Fixed scope of moved symbol 5057c57 Merge pull request diffblue#1557 from janmroczkowski/janmroczkowski/further-improvements-to-unified_difft 5e067bf Merge pull request diffblue#1481 from andreast271/do-c++-regression c9b6c42 Merge pull request diffblue#1513 from romainbrenguier/feature/input-string-printable c4486f1 Merge pull request diffblue#1552 from thk123/feature/goto-functions-utilities 2648cbb Make unified_difft::lcss return by value cd1258a Merge pull request diffblue#1425 from romainbrenguier/feature/java_new_array_data 6e3a0b0 Make more member function static 9efb65c Merge pull request diffblue#1556 from diffblue/revert-1554-janmroczkowski/more-static-member-functions-in-unified_difft 1c96ae5 Revert "Make more member function static in unified_difft" 9cb4569 Amend doxygen comments 4550676 Added missing utilities to the Makefile 7938bac Correcting linting errors 25d765b Use a for loop rather than chained algorithms e67d229 Renamed find declaration method fa14b47 Renamed utility file to require_goto_statements a657ec1 Moved functions into a namespace and documented them b96199f Moved and simplified the code for finding sub statements b9914a8 Add some java testing utilities. 2c175bd Update load_java_class to construct the entry point function 3453a89 Merge pull request diffblue#1554 from janmroczkowski/janmroczkowski/more-static-member-functions-in-unified_difft feaa85f Merge pull request diffblue#1455 from romainbrenguier/doc/string-solver-documentation c5ab866 Merge pull request diffblue#1430 from romainbrenguier/refactor/gather_indices fac9dea Rename "#lva_mode" to "lvsa_mode" 72c8533 Make two irep IDs 55b6ac5 Merge pull request diffblue#1502 from tautschnig/merge-failed-tests-printer dfa2ed2 Make more member function static d378980 Style: Disabling clang-format in get f5991ee Refactor universal_only_in_index to use expression iterators 9d1aa99 Correct constraints added for char_set e125e8a Refactor gather_indices to use for_each instead of visitor 4b0e2d4 Create goto-gcc symlink in cmake builds and enable goto-gcc tests 7736672 Style: use NOLINTNEXTLINE to avoid cpplint errors on long links 6016bef Improve readability of code imported from failed-tests-printer.pl dd6e431 test.pl: Use native perl instead of "cat" to print log file 3321735 Move implementation of failed-tests-printer.pl into test.pl ba16006 Do not use shell built-ins 96e169a Use single quotes for Windows compatibility d2c3752 Remove string_printable option from the solver b0de0e3 Test for string printable option on input strings 4b36fc6 Merge pull request diffblue#1533 from mgudemann/fix/support_class_bounds_generics 35096b8 Initialize architecture in `instantiate_not_contains` unit test b25630a Merge pull request diffblue#1550 from chrisr-diffblue/cleanup/java-generics-test-helpers 542a26d Stop adding printable constraints on all strings e65e340 Use command line option for string-printable param 8e92362 Propagate string-printable option in object_factory ae5f32e Add a printable option to string initialization 514e6a1 Add function to call constrain_character primitive 1d92c48 Add string primitive to constrain characters cb01526 Minor refactoring in add_default_axioms e1280cc Add utility function add_constraint_on_characters 6b88eb8 Add unit test for class / interface bound 2ed059a Support interface and class bound parsing in generics ccdd483 Merge pull request diffblue#1545 from chrisr-diffblue/TG-1158/unit-test-for-specialising-with-array-types 73808aa Merge pull request diffblue#1544 from smowton/smowton/feature/value_set_eq_operator 0507355 Refactored unit test helpers to be more general and extend their use-cases 93ebb84 Merge commit '356aed461b387a8ae815a9901a16d26f32f102be' into develop db758fb Add some unit test helper functions, useful for Java generics unit tests 98de899 Add a unit test for specialising Java generic types with array types b07fcdd Documentation improvements and readme for strings 1fa64a9 Avoid using is_valid_java_array in builin_functions 0dafac2 Add unit test for goto_trace_output in Makefile 435958f Unit test for goto_trace::output 5a0343f Doc: Summary for count_type_leaves fc363b3 Typo in goto_trace output 42c079d Use existing function for checking object is array 465e5dc Style: improve documentation in interpreter evaluate fe2efa7 Style: Replace assert by appropriate macros e36d7d8 Check if object is nil before writing trace 6b519ad Add identifier and rename statement to java_new_array_data d4f1b29 Add eq and neq operators to value_sett and related types b03ec16 Merge pull request diffblue#239 from diffblue/bugfix/value_sets_fi_and_reaching_defs_retrievals_of_dynamic_objects db79106 Added explanatory comment for the introduced condition. dfc6a20 Fixing C++ code-style issues. b0742cf Disable cbmc-cpp tests in appveyor, which runs regression on windows. All cbmc-cpp tests #include <assert.h> and cbmc cannot yet parse Microsoft C++ headers. d55a8da Add tests to cmake regression: cbmc-cover, cbmc-cpp, goto-analyzer-taint 3a4e48c Run cbmc c++ regression as part of default regression test Set is_parameter for c++ function parameter symbol 7989831 Added regression test for the fixed bug. 00b4af2 Bugfix: Explicit retrievals of DOs from value_set amd reaching_defs. git-subtree-dir: cbmc git-subtree-split: e8b3cb9
Cleaning, extending and adding new unit tests. Since there is no limit to writing unit tests, I expect this does not cover all that you may have in mind and thus any suggestions for more tests are welcome.