tests/context: drain generator return so pytest doesn't warn

[omegajudith]

Fixes #4618.

Pytest raised PytestReturnNotNoneWarning when a test returned a generator.
with_phases was returning the inner generator; this drains it and returns
None so pytest sees a proper test function.

Repro before:
pytest -W error -x -vv tests/core/pyspec/eth2spec/test/altair/light_client

After fix:
12 passed, 0 warnings.

[bomanaps]

Hello @jtraglia please could you help us review this?

[jtraglia]

Hey @omegajudith & @bomanaps this does technically fix the issue (by hiding the warnings), but it's not the proper fix. This will break the reference test generation side of things. For example, this works without warnings:

make test fork=altair k=test_light_client_data_collection

But this will now fail:

make reftests fork=altair k=test_light_client_data_collection

[jtraglia]

I believe the correct solution is to change the decorator order so that @spec_state_test_with_matching_config happens after @with_config_overrides, so that it handles the yield statements appropriately. Note that decorators are called bottom to top, closest to the function definition first.

consensus-specs/tests/core/pyspec/eth2spec/test/altair/light_client/test_data_collection.py

Lines 28 to 37 in aae5237

	@with_light_client
	@with_config_overrides(
	{
	"BLOB_SCHEDULE": sample_blob_schedule(initial_epoch=1, interval=1),
	},
	emit=False,
	)
	@spec_state_test_with_matching_config
	@with_presets([MINIMAL], reason="too slow")
	def test_light_client_data_collection(spec, state):

To be clear, this:

 @with_light_client
+@spec_state_test_with_matching_config 
 @with_config_overrides( 
     { 
         "BLOB_SCHEDULE": sample_blob_schedule(initial_epoch=1, interval=1), 
     }, 
     emit=False, 
 ) 
-@spec_state_test_with_matching_config 
 @with_presets([MINIMAL], reason="too slow") 
 def test_light_client_data_collection(spec, state): 

This will eventually call @spec_test and then @vector_test:

consensus-specs/tests/core/pyspec/eth2spec/test/context.py

Lines 322 to 328 in aae5237

	def spec_test(fn):
	# Bls switch must be wrapped by vector_test,
	# to fully go through the yielded bls switch data, before setting back the BLS setting.
	# A test may apply BLS overrides such as @always_bls,
	# but if it yields data (n.b. @always_bls yields the bls setting), it should be wrapped by this decorator.
	# This is why @always_bls has its own bls switch, since the override is beyond the reach of the outer switch.
	return vector_test()(bls_switch(fn))

This decorator will handle this situation here:

consensus-specs/tests/core/pyspec/eth2spec/test/utils/utils.py

Lines 61 to 73 in aae5237

	# check generator mode, may be None/else.
	# "pop" removes it, so it is not passed to the inner function.
	if kw.pop("generator_mode", False) is True:
	# return the yielding function as a generator object.
	# Don't yield in this function itself, that would make pytest skip over it.
	return generator_mode()
	else:
	# Just complete the function, ignore all yielded data,
	# we are not using it (or processing it, i.e. nearly zero efficiency loss)
	# Pytest does not support yielded data in the outer function, so we need to wrap it like this.
	for _ in fn(*args, **kw):
	continue
	return None

Please review/update all instances of @with_config_overrides and correct this order issue.

This issue was not as easy and I thought it would be. Sorry about that.

[leolara]

@jtraglia the yields when not in generation mode are consumed here: https://github.com/ethereum/consensus-specs/blob/master/tests/core/pyspec/eth2spec/test/utils/utils.py#L68-L73

in vector_test decorator. I think all tests that yield should have this decorator.