op-e2e: allow external processes to skip tests #7065
Conversation
|
I like the idea of the JSON file to define skipped tests - it also gives us a good place to put other configuration that may be required in the future. What do you think about requiring the JSON config to exist and have it specify the binary to execute? Probably not worth it if it complicates the implementation too much but it would give more flexibility and I think make things a bit more discoverable rather than having to know the convention that Will look through the code properly after the morning meeting gauntlet. :) |
|
Code looks good to me, but in a new twist CI doesn't appear to have run at all. I'd be ok with landing this as-is if you prefer but I lean a bit towards specifying the binary to run in the json file. Let me know what you think. |
|
I'd considered specifying the binary location via the JSON, but decided against it primarily because it simplified the change and because I couldn't think of a strong reason why anyone would want/need to pick a different path to the binary. From what I've seen in other places (like say, the devnet tests) the code tends to expect a single 'root' path from which other paths are derived. That being said, I don't think it would be particularly difficult to implement. If after some thought and review you'd still like the shim path in the JSON, let me know and I'll spend the few minutes to support it. |
|
I guess you always want to write a wrapper program to handle writing the ports file etc so the binary you point to would never be the actual client binary anyway. So yeah, let's just stick with it this way. Maybe try pulling into the latest changes from develop and see if that kicks circleci into action? |
|
Looks like our comments ended up passing each-other on the wire, so I hadn't read your reply yet. As far as the missing CI, I know Circle was having some issues with degraded service today, because of webhook problems of some sort, so maybe that's what's going on. With respect to the shim path -- how would you like the path to be specified? Should it be a relative path to the directory containing the JSON file (what seems most intuitive to me), or, relative to the |
Certain existing e2e tests are not adaptable to external binary testing. Although it's better for tests to be able to execute against both an in-process geth or an extra-process arbitrary ethereum client, the current state of the tests does not allow this in all cases. This change allows for external binaries to specify a set of test names to be skipped, including a skip message for why they are skipped. The set is specified as a JSON map in a new optional file `test_parms.json` which can be created in the external adapter folder. Prior to this change, the external process was indicated only by a path to a `shim` binary. This change now instead asks that the external test be specified by directory, wherein a `shim` binary must exist, and this new file `test_params.json` may exist. Other approaches are of course possible, such as allowing the JSON file to be specified via a command flag, but it seemed better to simply establish a convention for expected paths for these external tests to satisfy. There actually is a single test which already has a `t.Skip()` based on the presence of the external L2 flag. I'd though these were all removed in the last PR, but this one actually is required for an external op-geth. So, this skip has been converted to the new method and acts as a nice test that it is working.
jyellick
force-pushed
the
e2e-externals-specify-test-skips
branch
from
f2dbb31
to
955efc1
Compare
|
oh I meant that I was agreeing with you and we should just stick with the convention of the binary always being called Looks like circleci got kicked appropriately so hopefully that all passes and we can get this merged in. |
OptimismBot
merged commit 57bf998
into
ethereum-optimism:develop
|
This PR has been added to the merge queue, and will be merged soon. |
|
This PR is next in line to be merged, and will be merged as soon as checks pass. |
Certain existing e2e tests are not adaptable to external binary testing. Although it's better for tests to be able to execute against both an in-process geth or an extra-process arbitrary ethereum client, the current state of the tests does not allow this in all cases.
This change allows for external binaries to specify a set of test names to be skipped, including a skip message for why they are skipped. The set is specified as a JSON map in a new optional file
test_parms.jsonwhich can be created in the external adapter folder.Prior to this change, the external process was indicated only by a path to a
shimbinary. This change now instead asks that the external test be specified by directory, wherein ashimbinary must exist, and this new filetest_params.jsonmay exist. Other approaches are of course possible, such as allowing the JSON file to be specified via a command flag, but it seemed better to simply establish a convention for expected paths for these external tests to satisfy.There actually is a single test which already has a
t.Skip()based on the presence of the external L2 flag. I'd though these were all removed in the last PR, but this one actually is required for an external op-geth. So, this skip has been converted to the new method and acts as a nice test that it is working.