-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(plugins/forks): Fix covariant markers, add selector
/marks
parameters
#762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey LGTM! Few small comments below. It's pretty heavy stuff 😆 but 💯 worth the overhead!
I like it (and think we can merge this as-is), but I'm not entirely sure we have the right interface for applying marks yet (these don't seem to used anywhere...yet).
For example, it's entirely possible to want to apply multiple marks to multiple parameters and, if I understand correctly, it's currently only possible to apply multiple marks to one of the parameters?
What I mean is something like either:
-
or,
@pytest.mark.with_all_tx_types( marks=[[pytest.mark.xfail], [pytest.mark.skip]], marks_selector=[lambda tx_type: tx_type == 1, lambda tx_type: tx_type == 2], )
-
i.e., Allow the user to define a function that returns the marks to be applied for each param.
def tx_types_marks(tx_type: int): if tx_type == 1: return [pytest.mark.xfail, pytest.mark.slow] if tx_type == 2: return [pytest.mark.skipif(...)] return [] @pytest.mark.with_all_tx_types(marks=tx_types_marks)
But neither of these solve the case for marking with_all_txtypes
and with_all_precompiles
and wanting to mark if tx_type == 2 and precompile == 1
? 😆
I pushed some additions to the tests, which I mainly made to help me get into the topic. In particular, I added a couple of test cases for marks
and marks_selector
cf f17a383.
Also, not necessary for this PR, but what do you think about moving these fork helper functions get_from_until_fork_set
, get_forks_with_no_parents
, get_forks_with_no_descendants
and get_last_descendants
from pytest_plugins.forks.forks
to ethereum_test_forks.helpers
? Or do you consider them too specific?
I really like number 2, I think it's better than the selector implemented in this PR. I'm gonna give this a try and see if I can implement it in this PR.
We'll think about something when we need this :P
Sounds good, will do it in this PR 👍 |
Co-authored-by: danceratopz <[email protected]>
7f85321
to
e8eedeb
Compare
selector
/marks
parameters
🗒️ Description
with_all
markerswith_all
markers that share a common parameter, e.g.with_all_call_opcodes
andwith_all_create_opcodes
shareevm_code_type
, therefore when used together, only test cases where theevm_code_type
is compatible with each marker (e.gOp.CALL + Op.CREATE
orOp.EXTCALL + Op.EOFCREATE
, but notOp.EXTCALL + Op.CREATE
)marks
andmarks_selector
keyword parameter in allwith_all
markers, this method has the disadvantage that the same set of markers needs to be used in all test cases where the marker is applied.🔗 Related Issues
None
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.