[CAPPL-735] Engine V2 execution phase (minimal impl) #17590
Conversation
bolekk
force-pushed
the
feature/CAPPL-735-execution-phase
branch
3 times, most recently
from
ddacd7f to
bf4850e
Compare
bolekk
changed the title
bolekk
force-pushed
the
feature/CAPPL-735-execution-phase
branch
from
bf4850e to
cb8d3c1
Compare
bolekk
force-pushed
the
feature/CAPPL-735-execution-phase
branch
from
cb8d3c1 to
36cbd6b
Compare
|
|
||
| require.NoError(t, engine.Close()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
super nit, could you add a second test that runs the test executable used to test the module? It'll ensure that there's no gaps to re-use it. I wouldn't remove this test, since it's easier to debug if things go wrong.
I don't use config in that test yet, but it probably should opt to soon.
There was a problem hiding this comment.
This test is blocked until SetCapabilityExecutor is implemented on the host. So I've split this test out to a separate ticket.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌1 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
| WorkflowExecutionTimeoutMs uint32 | ||
| CapabilityCallTimeoutMs uint32 |
There was a problem hiding this comment.
Why not use time.Duration for these? Sheds the conversion on use, and makes the literals more readable.
There was a problem hiding this comment.
+1, for context, the other variables are already labeled Ms and uint32.
There was a problem hiding this comment.
this pattern is used throughout the capabilities and gateway packages, can move to a separate discussion because changing here makes this now inconsistent with other areas of the code.
|
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌1 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
No description provided.