Improve test decorators

[leolara]

Fixes #4618 - Warnings when running tests
Fixes #4659 - decorator vector_test has parameter description is never used
Fixes #4661 - decorator with_config_overrides parameter emit is unnecesary

Note: we should run the reftests on this before merging.

[leolara]

This works:

make reftests fork=altair k=light_client

And

make test k=test_light_client

Doesn't produce warnings

[jtraglia]

How is this a better solution than what I described here? This feels very complicated in comparison to just changing the order of decorators.

• Fix decorator order in light-client tests #4652 (comment)

[leolara]

How is this a better solution than what I described here? This feels very complicated in comparison to just changing the order of decorators.

• tests/context: drain generator return so pytest doesn't warn  #4652 (comment)

The code is wrong in python terms, we need to fix it, but I will do it with unit test so we make sure we are not introducing new bugs.

[leolara]

So, the best way of action IMHO is to fix this and also:

#4661
#4659

[jtraglia]

The code is wrong in python terms, we need to fix it

Please (reasonably) expand. I don't understand how it is wrong.

From Gemini:

No, a single Python function cannot sometimes be a generator and sometimes return a value in the way you might be thinking. The presence of the yield keyword within a function's definition fundamentally changes its nature.
Here's why:
Generator Functions: If a function contains the yield keyword anywhere in its body, Python automatically treats it as a generator function. When you call a generator function, it does not execute its code immediately and return a value. Instead, it returns a generator object (an iterator) that can be iterated over to produce a sequence of values.
Normal Functions: A function without yield statements is a normal function. When called, it executes its code and, if an explicit return statement is encountered, it returns the specified value (or None implicitly if no return statement is present or if return is used without a value).

So I think this code assumes that if you do not yield in one of the exuction paths, it becomes a normal function, but actually it becomes a generator that will produce an empy list.

Hence, you need two use different wrappers if you want to returns sometimes a generator and sometimes you want to return a non-generator. So a function can return some times a function that is a generator, and sometimes a function that is not a generator. But a function itself it is always either a generator or not.

I found about this writing this PR: #4440

here: https://github.com/ethereum/consensus-specs/pull/4440/files#diff-3e0bfbacc4b2ed6a8f4e592f6948136bd4792224c75b3b283947fae57271dcf8R23-R64

and the unit tests, allow me to see this more clearly.

[jtraglia]

@leolara next time please just post a new comment instead of editing mine.

I'm just unsure of this PR because I don't fully understand what's going on here.

Instead of creating a new function, why not just change the out block? Eg something like:

if emit:
  if out is not None:
    yield from out
else:
   return out

[jtraglia]

@leolara & I chatted about this off GitHub and it all makes sense now. I've merged #4652 but I want to merge this PR as well. Leo, please tag me when this is ready for review.

[leolara]

@jtraglia I think it is ready but we should not merge without running reftests in this branch

[jtraglia]

@jtraglia I think it is ready but we should not merge without running reftests in this branch

The reference test generator is fixed. Please test this & report back.

[leolara]

Running here: https://github.com/ethereum/consensus-specs/actions/runs/18619818770

[jtraglia]

Running here: https://github.com/ethereum/consensus-specs/actions/runs/18619818770

Hmm this isn't right. This is for a1ce256f on master, not this branch.

[leolara]

Running here: https://github.com/ethereum/consensus-specs/actions/runs/18619818770

Hmm this isn't right. This is for a1ce256f on master, not this branch.

As by this screenshot of this link, it is:

[jtraglia]

Running here: https://github.com/ethereum/consensus-specs/actions/runs/18619818770

Hmm this isn't right. This is for a1ce256f on master, not this branch.

As by this screenshot of this link, it is:

Ah yes. I misunderstood what the commit on the main page meant. It must be the workflow commit.

[jtraglia]

The core changes look fine to me. I assume most of the new infra tests are AI generated. I skimmed them and they seem decent enough.