[tarantool-patches] Re: [PATCH 3/5] test: speed up swim big cluster failure detection

[Vladislav Shpilevoy]

On 09/04/2019 11:43, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [19/04/05 16:12]:
> 
> The more I look at the tests the more confusing the test function
> names become. 
> 
> The namespace of test functions is clearly clashing with the main
> swim namespace, which makes the tests hard to read and follow:
> 
> it's unclear which function belongs to the tests harness and which
> to the swim itself. Please come up with a harness api prefix.

Short answer: 'swim_cluster_' is the harness API prefix. There are
only two harness' public functions not prefixed: swim_run_for() and
swim_error_check_match(), but they are just utilities, and not
related to a concrete set of swim instances (cluster). I've fixed
these two functions to be prefixed with 'swim_test_'.

Long answer below.

> 
> Please feel free to do it in a subsequent patch.

I do not understand, how is it supposed to help, because it is not
necessary to know which function are public, and which are not, to
understand what happens in the test. My logic is that a test should
be easy to read like a text, and to understand the scenario. And for
that we usually never prefix our test functions with special prefixes.
Even in unit tests. You still continue to create new rules so as to
forget about them afterwards, and create contradictory ones or just
break olds. And you never document any of your ideas.

Below you can find some examples of not prefixing test functions.
Some of them are simple, some of them (just like swim) are a part of
a harness.

Examples:
- unit/bitset_iterator.c, test functions, not prefixed:
  * bitsets_create
  * bitsets_destroy
  * nums_fill
  * nums_comparator
  * nums_sort
  * nums_shuffle

- unit/cbus.c, test functions, not prefixed:
  * do_nothing
  * flush_cb
  * finish_execution
  * worker_f
  * worker_start
  * worker_stop
  * do_forced_flush
  * do_some_event

- unit/cbus_stress.c, test functions and structures, not prefixed:
  * struct conn
  * struct thread
  * thread_create, thread_name, thread_func, thread_destroy,
    thread_connect, thread_disconnect, thread_connect_random,
    thread_disconnect_random, thread_send, thread_send_random
  * struct thread_msg
  * thread_msg_received_cb

  By the way, that test was authored by Vova and *committed by you*.

- unit/column_mask.c, test functions and structures, not prefixed:
  * struct tuple_op_template
  * struct tuple_update_template
  * struct tuple_template
  * tuple_new_raw
  * tuple_new_update
  * tuple_update_alloc_f
  * check_update_result
  
  That test was authored by me and *committed by you*.

- unit/csv.c, test functions and structures, not prefixed:
  * print_endl
  * print_field
  * buf_endl
  * buf_field
  * struct counter
  * line_counter
  * fieldsizes_counter
  * csv_out

- unit/fiber.c, test functions, not prefixed:
  * noop_f
  * cancel_f
  * exception_f
  * no_exception_f
  * cancel_dead_f
  * stack_expand
  
  Here the names were invented or patched *by you*.

- unit/histogram.c, test functions, not prefixed:
  * int64_cmp
  * int64_sort
  * gen_buckets
  * gen_rand_data
  * gen_rand_value

unit/say.c, test functions and structures, not prefixed:
  * parse_logger_type
  * parse_syslog_opts
  * format_func_custom
  * struct create_log
  * dummy_log

  These parts of the test were authored by Ilya and
  *committed by you*.

My favorite part - Vinyl. It is relatively new, and was
almost completely reviewed and committed by you.

unit/vy_iterators_helper.c/.h, test functions and structures,
not prefixed:
  * vy_new_simple_stmt
  * vy_mem_insert_template
  * vy_cache_insert_templates_chain
  * vy_cache_on_write_template
  * init_read_views_list
  * vy_stmt_are_same
  * struct vy_stmt_template

unit/vy_log.stub.c, test functions, not prefixed:
  * vy_log_next_id
  * vy_log_tx_begin
  * vy_log_tx_commit
  * vy_log_write
  * vy_recovery_lsm_by_index_id

unit/vy_point_lookup.c: write_run().

I skipped most of the small test files, and decided not
to scan non-unit tests. But apparently in non-unit tests
we never use prefixes as well. And evidently this lack of
useless padding-out-the-code prefixes does not aggravate
readability of the tests.

What is more, even now all non-test functions can be
identified by the first 'struct swim *' and
'struct swim_member *' parameters. Test functions are
prefixed with 'swim_cluster_' prefix. The only methods, not
prefixed with 'swim_cluster_' here were 'swim_run_for'
and 'swim_error_check_match'.

I renamed these two methods with a prefix 'swim_test_', and
changed 'swim_start/finish_test' macro to 'swim_test_start/finish'
to be consistent. Now *all* public test harness methods are
either prefixed with 'swim_cluster_' or with 'swim_test_'.

I will not send it as a follow-up patch in order to keep git
history as clean as possible. Only old tests I will fix
separately, before the main patch.