More tests, docs, and minor things #41
Conversation
# Conflicts: # tests/worker/test_workflow.py
cretz
force-pushed
the
more-tests
branch
2 times, most recently
from
69dc4cf to
0355f47
Compare
|
|
||
| #### Testing | ||
|
|
||
| Tests currently require [Go](https://go.dev/) to be installed since they use an embedded Temporal server as a library. |
There was a problem hiding this comment.
Are you going to get rid of this soon?
There was a problem hiding this comment.
There are two things in Go right now: Temporalite wrapper and "kitchen-sink" workflow/worker. I am waiting on TLS support and distributed Temporalite binaries for the former. For the latter, probably at the same time I get rid of the former, I will rewrite the workflow in Python and get rid of the Go worker.
I could move to docker compose I suppose, though Temporalite is very fast for our use case.
| @@ -35,6 +37,21 @@ class RetryPolicy: | |||
| non_retryable_error_types: Optional[Iterable[str]] = None | |||
| """List of error types that are not retryable.""" | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
nit: could use a classmethod instead and instaniate cls() below instead of referencing the class by name
There was a problem hiding this comment.
I like being explicit on class instantiation here (and I have chosen to use @staticmethod throughout, though Google style guide wants @classmethod; I have documented the deviation in the README too)
| # Mark it as started each loop because backoff could cause it to | ||
| # be marked as unstarted | ||
| handle._started = True |
There was a problem hiding this comment.
Is this referring to local activities?
There was a problem hiding this comment.
Yes, but we share a good bit of code w/ local/non-local activity handling so it's in this shared space
| if hasattr(task, "_log_destroy_pending"): | ||
| setattr(task, "_log_destroy_pending", False) |
There was a problem hiding this comment.
Should we only do this for workflows that we know we've evicted or after shutdown?
There was a problem hiding this comment.
I think this only applies in those two cases since this is only used on GC and I hold a strong reference until eviction/shutdown or the task is done. I just choose to do this at registration time instead of loop over all tasks at the end.
| @workflow.run | ||
| async def run(self) -> None: | ||
| # Wait forever | ||
| await asyncio.Future() |
There was a problem hiding this comment.
Wondering if it's worth having a workflow method to "block" like this
There was a problem hiding this comment.
I am not sure a "block forever" is a primitive we need in any lang. I suppose it's just await new Promise(() => {}) in TS which I figure we don't have a utility for. I guess you could await workflow.wait_condition(lambda: false) too.
There was a problem hiding this comment.
Yeah, in TS you have to use await new Trigger() or await CancellationScope.current().cancelled or await condition(() => false).
There's no exported function and I have gotten questions on how to achieve this.
I recommend waiting on the cancellation scope directly.
There was a problem hiding this comment.
I think sleeping until cancel is a bit of an anti-pattern due to history growth, though some people trust they'll stay low until they cancel. Python doesn't have an explicit wait forever so not sure we need one. They could just as easily do await asyncio.Event().wait() or anything like that.
|
|
||
| @workflow.query | ||
| def bad_query(self) -> NoReturn: | ||
| raise ApplicationError("query fail", 456) |
There was a problem hiding this comment.
nit: don't need an ApplicationError here
There was a problem hiding this comment.
We do if I want to test how details are handled :-) Though I note "(no details on query failure)" in the test, so it became useless.
| assert isinstance(err.value.cause, ApplicationError) | ||
| assert list(err.value.cause.details) == [123] | ||
| # Fail query (no details on query failure) | ||
| with pytest.raises(RPCError) as rpc_err: |
There was a problem hiding this comment.
We wrap query failures in other SDKs with QueryRejectedError
There was a problem hiding this comment.
Actually, at least in TS that's only on successful query response with a query_rejected field which is different than this failure.
But yes, in other SDKs we do sometimes make the RPC errors more palatable in some cases. But some SDKs get it wrong (e.g. assuming not found means workflow not found). This was discussed at #32 (comment) and temporalio/features#59 was opened as a result.
| await assert_eq_eventually(True, child_started) | ||
| # Send cancel signal and wait on the handle | ||
| await handle.signal(CancelChildWorkflow.cancel_child) | ||
| await handle.result() | ||
| assert isinstance(err.value.cause, ChildWorkflowError) |
There was a problem hiding this comment.
Pardon my laziness, remind me what err.value is here? Is it the Failure object?
There was a problem hiding this comment.
It's the Python Exception. Pytest wraps it, so err here is technically https://docs.pytest.org/en/7.1.x/reference/reference.html#exceptioninfo
| event.timer_started_event_attributes.start_to_fire_timeout.ToMilliseconds() | ||
| == 10 |
There was a problem hiding this comment.
Hmm.. looking at this now I'm wondering if it's worth not sending this command if it's cancelled in the same activation. (Would be a backwards incompatible Core change).
There was a problem hiding this comment.
From what I saw in history, it seems core properly handles cancelling out a command sent in the same activation completion. It's for that reason I didn't add extra code to remove commands of things I cancel in the same completion before sending the completion - it seems like core does it.
| assert 1 == sum( | ||
| 1 |
@bergundy - There are probably some I missed, but can you point me at them again just so I have them tracked? |
|
Did you check all interceptors e2e? |
|
I will add tests for those two EDIT: Only adding test for the first one for now and will do the next in another PR (after off-PR discussions about how best to solve). |
What was changed
Checklist