Re: [PATCH] trace2 tests: guard pthread test with "PTHREAD"

[Jeff Hostetler <git@jeffhostetler.com>]

On 11/28/22 12:50 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Nov 28 2022, Jeff Hostetler wrote:
> 
>> On 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:
>> [...]
>>> That was my thought as well, but these tests are specifically testing
>>> how it interacts with threading. The counter mechanism works in general
>>> without threading.
>>> The test descriptions don't help, and should really say that they're
>>> to
>>> do with threading in particular, but I wanted to keep this as small as
>>> possible for rc[12] and the final, so I didn't fix that while-at-it.
>>
>> There is large comment block above `have_timer_event()` and
>> `have_counter_event()` in t0211 that explained the purpose of the
>> test1 and test2 tests for each.  Would it help if that text were moved
>> down before each of the individual tests rather than where it is now?
> 
> You did ask :)

Yes, I did.  :-) :-)


> I think that better than a comment is to have the test description
> itself reflect the nature & purpose of the test.
> 
> Now the two are:
> 
> 	test_expect_success 'global counter test/test1' '
> 	test_expect_success PTHREAD 'global counter test/test2' '
> 
> So at least the PTHREAD shows that it's something to do with threading,
> but if it fails with that prereq passed you'll need to consult the
> source to see what "test2" is supposed to do. Better would be to just
> skip the comment & work "single threaded" and "multi-threaded" etc. into
> the test name itself.
> 
> Ditto symbol names "ut_200counter" and "ut_201counter", again, a comment
> somewhere in t/helper/test-trace2.c notes that they're single- and
> multi-threaded, respectively, but why not skip that and make the symbol
> names self-descriptive?

Good points. I'll keep this in mind for the future. I don't
think I want to send a fix for them now, but maybe the next
time I'm in the area...


Thanks again,
Jeff