Fix decorator order in light-client tests

[omegajudith]

Fixes #4618.
Reorders decorators so @spec_state_test_with_matching_config appears above
@with_config_overrides in Altair light-client tests.

Files touched:

• tests/core/pyspec/eth2spec/test/altair/light_client/test_data_collection.py
• tests/core/pyspec/eth2spec/test/altair/light_client/test_sync.py
• tests/core/pyspec/eth2spec/test/altair/unittests/light_client/test_sync_protocol.py
• had 11 files but all 3 needed the required modification

Rational:

• Matches the order expected by the test utils so yields/overrides are handled correctly.

Test plan:

• Local smoke test of the affected area (optional):
  make venv && source .venv/bin/activate
  make test PYTEST_ARGS='-q -k "light_client and altair"'
• Full CI will run on this PR.

[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.

[leolara]

In my opinion this is the solution: #4658

[leolara]

@jtraglia

make reftests fork=altair k=test_light_client_data_collection

This cannot ever run a test, because the behavoiur of k in reftests is different than in test, and you need to remove test_ for reftests.

[jtraglia]

@jtraglia

make reftests fork=altair k=test_light_client_data_collection

This cannot ever run a test, because the behavoiur of k in reftests is different than in test, and you need to remove test_ for reftests.

Sorry, this is just a typo on my part. Look at the screenshot.

[leolara]

@jtraglia

make reftests fork=altair k=test_light_client_data_collection

This cannot ever run a test, because the behavoiur of k in reftests is different than in test, and you need to remove test_ for reftests.

Sorry, this is just a typo on my part. Look at the screenshot.

Sorry, didn't read the screenshot, this is a clarification because some times it caused me problems. Not a priority but it would be good if k works with test_ in the reftests

[jtraglia]

Let's also revert the changes to this file.

[omegajudith]

hey @jtraglia and @leolara

Per your guidance, this PR only reorders decorators so
@spec_state_test_with_matching_config sits above @with_config_overrides
in the Altair light-client tests (3 files). No other changes.

[jtraglia]

LGTM, thank you @omegajudith & @bomanaps!

[etan-status]

It seems that the blob schedule is missing in the generated config.yaml, so now tests fail when trying to run in an external runner (that consumes the generated vectors).

[etan-status]

[etan-status]

Note: The blob schedule was there in beta.0, it seems something about the decorator order change broke this.