Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Jun 6, 2024, 23:35 UTC
Message-ID: <dohbd64jxuahelut63esztozdozqrhx5rgv5m4t3wt5gz6v6kv@6q2aivlcvxcq>
In-Reply-To: <xmqqo78dka99.fsf@gitster.g>

On Thu, 06 Jun 2024, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > +static void check_each(struct oidtree *ot, char *hex, char *expected)
> > +{
> > +	struct object_id oid;
> > +
> > +	if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
> > +		return;
> > +	oidtree_each(ot, &oid, 40, check_each_cb, expected);

I think I mistakenly kept '40' from when I was testing, but it should be strlen(hex). Will correct.

Show 35 quoted lines

> > +static void t_each(struct oidtree *ot)
> > +{
> > +	FILL_TREE(ot, "f", "9", "8", "123", "321", "a", "b", "c", "d", "e");
> > +	check_each(ot, "12300", "123");
> > +	check_each(ot, "3211", ""); /* should not reach callback */
> > +	check_each(ot, "3210", "321");
> > +	check_each(ot, "32100", "321");
> > +}
> 
> Testing "each" with test data that yields only at most one response
> smells iffy.  It is a problem in the original test, and not a
> problem with the conversion, ...
> 
> BUT
> 
> ... in the original, it is easy to do something like the attached to
> demonstrate that "each" can yield all oid that the shares the query
> prefix.  But the rewritten unit test bakes the assumption that we
> will only try a query that yields at most one response into the test
> helper functions.  Shouldn't we do a bit better, perhaps allowing the
> check_each() helper to take variable number of parameters, e.g.
> 
> 	check_each(ot, "12300", "123", NULL);
> 	check_each(ot, "32", "320", "321", NULL);
> 
> so the latter invocation asks "ot" trie "I have prefix 32, please
> call me back with each element you have that match", and makes sure
> that we get called back with "320" and then "321" and never after.
> 
> Come to think of it, how is your check_each_cb() ensuring that it is
> only called once with "123" when queried with "12300"?  If the
> callback is made with "123" 100 times with the single query with
> "12300", would it even notice?  I would imagine that the original
> would (simply because it dumps each and every callback to a file to
> be compared with the golden copy).

That's true! I did not think of that. What do you think about something like this then? I will clean it up to send in v2.

---

struct cb_data {
	int *i;
	struct strvec *expected_hexes;
};

static enum cb_next check_each_cb(const struct object_id *oid, void *data)
{
	struct cb_data *cb_data = data;
	struct object_id expected;

	if(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {
		test_msg("error: extraneous callback. found oid: %s", oid_to_hex(oid));
		return CB_BREAK;
	}

	if (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))
		return CB_BREAK;
	if (!check(oideq(oid, &expected)))
		test_msg("expected: %s\n       got: %s",
			 hash_to_hex(expected.hash), hash_to_hex(oid->hash));

	*cb_data->i += 1;
	return CB_CONTINUE;
}

static void check_each(struct oidtree *ot, char *query, ...)
{
	struct object_id oid;
	struct strvec hexes = STRVEC_INIT;
	struct cb_data cb_data;
	const char *arg;
	int i = 0;

	va_list expected;
	va_start(expected, query);

	while ((arg = va_arg(expected, const char *)))
		strvec_push(&hexes, arg);

	cb_data.i = &i;
	cb_data.expected_hexes = &hexes;

	if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
		return;
	oidtree_each(ot, &oid, strlen(query), check_each_cb, &cb_data);

	if (!check_int(*cb_data.i, ==, cb_data.expected_hexes->nr))
		test_msg("error: could not find some oids");
}
---

Thanks for the review.