git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Achu Luma" <ach.lumap@gmail.com>, "Rubén Justo" <rjusto@gmail.com>
Cc: git@vger.kernel.org,  christian.couder@gmail.com,
	 Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
Date: Fri, 12 Jan 2024 14:44:37 -0800	[thread overview]
Message-ID: <xmqqmsta6uju.fsf@gitster.g> (raw)
In-Reply-To: <20240112102122.1422-1-ach.lumap@gmail.com> (Achu Luma's message of "Fri, 12 Jan 2024 11:21:22 +0100")

Achu Luma <ach.lumap@gmail.com> writes:

> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for advise_if_enabled functionality
> from the legacy approach using the test-tool command `test-tool advise`
> in t/helper/test-advise.c to the new unit testing framework
> (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).

In the light of the presense of work like this one

  https://lore.kernel.org/git/c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com/

I am not sure if moving this to unit-test framework is a good thing,
at least right at this moment.

To test the effect of setting one configuration variable, and ensure
it results in a slightly different advice message output to the
standard error stream, "test-tool advice" needs only a single line
of patch, but if we started with this version, how much work does it
take to run the equivalent test in the other patch if it were to be
rebased on top of this change?  It won't be just the matter of
adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
will it?

I doubt the issue is not about "picking the right moment" to
transition from the test-tool to unit testing framework in this
particular case.  Is "check-advice-if-enabled" a bit too high level
a feature to be a good match to "unit" testing, I have to wonder?

Thanks.


  reply	other threads:[~2024-01-12 22:44 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-12 10:21 [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c Achu Luma
2024-01-12 22:44 ` Junio C Hamano [this message]
2024-01-15 11:37   ` Rubén Justo
2024-01-15 17:27     ` Junio C Hamano
2024-01-15 19:24       ` Rubén Justo
2024-01-16 11:30         ` Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmsta6uju.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=ach.lumap@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=rjusto@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).