Skip to content

User-facing TestWorkflowRule doesn't enforce test timeouts anymore#719

Merged
Spikhalskiy merged 1 commit into
temporalio:masterfrom
Spikhalskiy:issue-634
Sep 16, 2021
Merged

User-facing TestWorkflowRule doesn't enforce test timeouts anymore#719
Spikhalskiy merged 1 commit into
temporalio:masterfrom
Spikhalskiy:issue-634

Conversation

@Spikhalskiy
Copy link
Copy Markdown
Contributor

@Spikhalskiy Spikhalskiy commented Sep 13, 2021

What was changed

TestWorkflowRule#setTestTimeoutSeconds is deprecated and transformed to noop.
Existing timeout logic is moved to SDKTestWorkflowRule and should be used only in internal Temporal tests.
Also moved handy newWorkflowStub(Class<T> workflow) and newUntypedWorkflowStub(String workflow) from SDK TestWorkflowRule to TestWorkflowRule, because they can be handy for our users.

Why?

As described in #634, Temporal Test rule shouldn't reimplement basic unit test framework functionality. And default timeout of 10 seconds could be annoying for users in some cases because users have to find a way to disable it.

  1. Closes Global timeout should be relocated from TestWorkflowRule to SDKTestWorkflowRule #634

@mfateev
Copy link
Copy Markdown
Member

mfateev commented Sep 14, 2021

Could we add a unit test that validates that JUnit way to enforce timeout works with TestWorkflowRule?

@Spikhalskiy
Copy link
Copy Markdown
Contributor Author

Spikhalskiy commented Sep 14, 2021

@mfateev wouldn't it be a unit test for junit? Ideally, junit rules should be orthogonal and independent.
Do you offer to test that

@Test(timeout=1000)
public void test() {
workflowThatExecutes2Seconds();
}

actually throws a timeout exception?

@mfateev
Copy link
Copy Markdown
Member

mfateev commented Sep 14, 2021

The reason TestWorkflowRule has this timeout is that it sets the default one to 10 seconds. You removed this timeout and the default. In the majority of cases, Temporal related unit tests don't simply fail but get stuck as it is a complex multithreaded application. So after this change almost any unit test that uses TestWorkflowRule is going to add the timeout to every test or test class.

@Spikhalskiy
Copy link
Copy Markdown
Contributor Author

Spikhalskiy commented Sep 14, 2021

@mfateev as they should if users want this behavior. I'm not sure if you were on a standup when I was discussing this change with folks, but the main trigger for it is the fact that users are actually annoyed by these 10 seconds, don't understand why we enforce it and they don't know that they can disable it. It’a especially annoying during debugging.
So, because it's a default functionality of any unit test framework, the idea is that we shouldn’t 1. Reimplement it 2. Providence default timeouts.

Also, from my experience, “stuck inside” is actually a rare scenario for java-sdk. I’m not sure I got it ever during App development other then “stuck inside after several hours of run”.

@Spikhalskiy Spikhalskiy force-pushed the issue-634 branch 2 times, most recently from 811025a to a1d426c Compare September 16, 2021 17:20
@Spikhalskiy
Copy link
Copy Markdown
Contributor Author

The workflow history got printed as before if we use standard JUnit ability to enforce timeout and the test fails/times out.

Comment on lines +97 to +108

globalTimeout =
!DebugModeUtils.isTemporalDebugModeOn()
? Timeout.seconds(
builder.testTimeoutSeconds == 0
? DEFAULT_TEST_TIMEOUT_SECONDS
: builder.testTimeoutSeconds)
: null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Swap the return values here so the condition isn't negative.

Comment on lines +212 to +223
* @deprecated Temporal test rule shouldn't be responsible for enforcing test timeouts. Use
* toolchain of your test framework to enforce timeouts.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should mention not only is this deprecated, it now does nothing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about deprecating it for now, but setting timeout only if the value is set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rolled back the removal of the actual implementation. Let's clean it out in 1.5

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class SDKTestWorkflowRule implements TestRule {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring about why you would use this over normal TestWorkflowRule would be useful

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in internal package, it's for us (SDK devs) only

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, still useful for new devs taking a look at it I mean.

Copy link
Copy Markdown
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Ultimately it seems reasonable to depend on existing frameworks for timeouts.

@Spikhalskiy Spikhalskiy merged commit 517aa52 into temporalio:master Sep 16, 2021
@Spikhalskiy Spikhalskiy deleted the issue-634 branch April 15, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Global timeout should be relocated from TestWorkflowRule to SDKTestWorkflowRule

3 participants