Adds isAborted method and joiners array property to mock task #2297

lourd · 2022-05-13T05:11:12Z

Q	A
Fixed Issues?	Fixes #2205
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	No new tests
Any Dependency Changes?

When doing saga integration testing (using expectSaga from redux-saga-test-plan), creating a mock task (passed into the saga using the fork.fn static providers) that's passed to a real join effect throws an error. Adding isAborted and the joiners array are necessary for it to not throw an error.

During code review, it was determined that createMockTask should have an API more similar to the real task implementation. setRunning was deprecated, and trying to change the state of a task more than once will now throw an error.

Also includes new unit tests for covering basic usage, and a small update to the docs with slightly improved instructions.

changeset-bot · 2022-05-13T05:11:14Z

🦋 Changeset detected

Latest commit: 9cc6729

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@redux-saga/testing-utils	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Andarist · 2022-06-06T08:47:13Z

packages/testing-utils/src/index.js

  let _result
  let _error

  return {
    [TASK]: true,
    isRunning: () => _isRunning,
+    isAborted: () => _isAborted,


This always returns a constant value - false. Shouldn't we expose a similar setter as we do for other "statuses" (setRunning, setError, etc)? I also wonder - some of those can't be true at the same time, so I think that it would make sense to create an internal status variable, initialize it to running' and then switch it appropriately based on those setters. That way we won't end up with an impossible task with t.isRunning() === true && t.isAborted() === true

In general, it's fairly easy to inspect how the real task behaves here:

redux-saga/packages/core/src/internal/newTask.js

Lines 118 to 141 in 4da7bd9

isRunning: () => status === RUNNING,

/*

This method is used both for answering the cancellation status of the task and answering for CANCELLED effects.

In most cases, the cancellation of a task propagates to all its unfinished children (including

all forked tasks and the mainTask), so a naive implementation of this method would be:

`() => status === CANCELLED || mainTask.status === CANCELLED`

But there are cases that the task is aborted by an error and the abortion caused the mainTask to be cancelled.

In such cases, the task is supposed to be aborted rather than cancelled, however the above naive implementation

would return true for `task.isCancelled()`. So we need make sure that the task is running before accessing

mainTask.status.

There are cases that the task is cancelled when the mainTask is done (the task is waiting for forked children

when cancellation occurs). In such cases, you may wonder `yield io.cancelled()` would return true because

`status === CANCELLED` holds, and which is wrong. However, after the mainTask is done, the iterator cannot yield

any further effects, so we can ignore such cases.

See discussions in #1704

*/

isCancelled: () => status === CANCELLED || (status === RUNNING && mainTask.status === CANCELLED),

isAborted: () => status === ABORTED,

result: () => taskResult,

error: () => taskError,

}

Sure, good point; updated the PR with your suggestion.

lourd · 2022-06-06T13:28:45Z

packages/testing-utils/types/index.d.ts

@@ -107,7 +107,8 @@ export interface SagaIteratorClone extends SagaIterator {
 export function createMockTask(): MockTask

 export interface MockTask extends Task {
-  setRunning(running: boolean): void
+  setRunning(): void


The refactor to a status enum required changing this type signature.

We could add a @deprecated signature back and handle both at the implementation level - task.setRunning() should just be synonymous with task.setRunning(true). I'm not 100% sure what task.setRunning(false) should correspond to though... it seems like canceled would fit better here than aborted

For all of the times I've used this in the past, I've always had to do:

mockTask.setResult(X) mockTask.setRunning(false)

This is necessary to get the rest of the saga to correctly proceed when joining on a mock task.

I've never had to do setRunning(true) because the tasks are already running when created.

I think taking out setRunning altogether is simpler than trying to guess what people would expect. Maybe that means publishing another major version? I think since it's a testing utility if we did it in a minor version it would be acceptable.

packages/testing-utils/types/index.d.ts

packages/testing-utils/src/index.js

Andarist · 2022-06-07T19:27:55Z

packages/testing-utils/src/index.js

-    setRunning: b => (_isRunning = b),
-    setResult: r => (_result = r),
-    setError: e => (_error = e),
+    setRunning: () => (status = RUNNING),


just a random thought without full analysis - perhaps setRunning should just be deprecated? it seems to me that making a mock task forcefully take a transition that isn't possible in the real code is asking for trouble.

Valid transitions are those:

running -> done (can be achieved by calling setResult)

running -> aborted (can be done by calling setError)

running -> canceled (could be done by cancel)

Thoughts?

I agree, I think that's a good idea.

Went ahead and added checks for that and tests to cover it :)

packages/testing-utils/src/index.js

lourd · 2022-06-11T19:47:49Z

@Andarist addressed all your comments and updated the PR description, PTAL.

Andarist · 2022-06-13T20:04:55Z

packages/testing-utils/types/index.d.ts

@@ -107,7 +107,6 @@ export interface SagaIteratorClone extends SagaIterator {
 export function createMockTask(): MockTask

 export interface MockTask extends Task {
-  setRunning(running: boolean): void


I totally agree with this change - I am concerned about this being a breaking change though. I'm not opposed to releasing a new major version of the Redux Saga or accompanying packages but this seems like a minor cleanup that doesn't necessarily require us to immediately bump a major version.

This PR will probably change the behavior of this task mock slightly, one way or another. But removing a known method is a big breaking change (for this package alone). Could we think about something less disruptive here? My preference would be to add this method back and introduce @deprecated + console.warn for what we want to phase out.

Ok, re-added with a deprecation warning.

Andarist · 2022-06-22T08:10:03Z

packages/testing-utils/src/index.js

-  let _isRunning = true
-  let _result
-  let _error
+  let status = RUNNING


nit: in general, it feels that keeping this as a number doesn't buy us anything here since we have to remap this later to string anyway when throwing an error. We could just ditch that if we'd use string status here.

I don't feel strongly about this though - both approaches are fine.

Andarist · 2022-06-22T08:16:11Z

packages/testing-utils/src/index.js

+      assertStatusRunning(status)
+      status = CANCELLED
+    },
+    joiners: [],


I have actually never used this mock task myself - it was here when I got involved in the project and I never bothered to tweak/remove it. I'm also very rusty when it comes to this codebase as I wasn't actively using it for a couple of last years. I wonder though - how useful this mock is in its current shape.

It seems to me that this now won't throw when passed to a join but at the same time the join effect will never get resolved.

I wonder if setError works OK when it comes to aborting the parent task etc. Do we have any tests for that? Does it work?

All of the new tests in this PR are pretty comprehensive IMO of describing what behavior works vs what doesn't. If you set the error before it's passed to a join effect it will correctly throw and abort the parent task.

Andarist

Just a changeset away from landing this :)

Adds isAborted and joiners to mock task

c9ce7f4

neurosnap added the enhancement label May 13, 2022

Andarist reviewed Jun 6, 2022

View reviewed changes

Refactors mock task internals to have status enum

96eb57d

lourd requested a review from Andarist June 6, 2022 13:27

lourd commented Jun 6, 2022

View reviewed changes

Andarist reviewed Jun 7, 2022

View reviewed changes

packages/testing-utils/types/index.d.ts Outdated Show resolved Hide resolved

Andarist reviewed Jun 7, 2022

View reviewed changes

packages/testing-utils/src/index.js Outdated Show resolved Hide resolved

Andarist reviewed Jun 7, 2022

View reviewed changes

packages/testing-utils/src/index.js Outdated Show resolved Hide resolved

lourd requested a review from Andarist June 11, 2022 19:40

Louis DeScioli added 2 commits June 11, 2022 18:19

Removes setRunning, ensures valid transitions, adds tests & updates docs

5c99ce1

Adds more cancel & join tests for mock tasks

13336e9

Andarist reviewed Jun 13, 2022

View reviewed changes

lourd requested a review from Andarist June 14, 2022 13:20

Re-adds setRunning with deprecation warning

30df6d9

Andarist reviewed Jun 22, 2022

View reviewed changes

Andarist approved these changes Jun 22, 2022

View reviewed changes

Creates changeset

9cc6729

neurosnap merged commit fdca798 into redux-saga:master Jun 27, 2022

lourd deleted the update-mock-task branch June 27, 2022 16:02

lourd mentioned this pull request Aug 11, 2022

Recent improvements not published #2319

Closed

This was referenced Aug 13, 2022

Version Packages #2280

Closed

Version Packages #2322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds isAborted method and joiners array property to mock task #2297

Adds isAborted method and joiners array property to mock task #2297

lourd commented May 13, 2022 •

edited

Loading

changeset-bot bot commented May 13, 2022 •

edited

Loading

Andarist Jun 6, 2022

Andarist Jun 6, 2022

lourd Jun 6, 2022

lourd Jun 6, 2022

Andarist Jun 7, 2022

lourd Jun 11, 2022

Andarist Jun 7, 2022

lourd Jun 11, 2022

lourd commented Jun 11, 2022

Andarist Jun 13, 2022

lourd Jun 14, 2022

Andarist Jun 22, 2022

Andarist Jun 22, 2022

lourd Jun 23, 2022

Andarist left a comment

	isRunning: () => status === RUNNING,
	/*
	This method is used both for answering the cancellation status of the task and answering for CANCELLED effects.
	In most cases, the cancellation of a task propagates to all its unfinished children (including
	all forked tasks and the mainTask), so a naive implementation of this method would be:
	`() => status === CANCELLED \|\| mainTask.status === CANCELLED`

	But there are cases that the task is aborted by an error and the abortion caused the mainTask to be cancelled.
	In such cases, the task is supposed to be aborted rather than cancelled, however the above naive implementation
	would return true for `task.isCancelled()`. So we need make sure that the task is running before accessing
	mainTask.status.

	There are cases that the task is cancelled when the mainTask is done (the task is waiting for forked children
	when cancellation occurs). In such cases, you may wonder `yield io.cancelled()` would return true because
	`status === CANCELLED` holds, and which is wrong. However, after the mainTask is done, the iterator cannot yield
	any further effects, so we can ignore such cases.

	See discussions in #1704
	*/
	isCancelled: () => status === CANCELLED \|\| (status === RUNNING && mainTask.status === CANCELLED),
	isAborted: () => status === ABORTED,
	result: () => taskResult,
	error: () => taskError,
	}

Adds isAborted method and joiners array property to mock task #2297

Adds isAborted method and joiners array property to mock task #2297

Conversation

lourd commented May 13, 2022 • edited Loading

changeset-bot bot commented May 13, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd commented Jun 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist left a comment

Choose a reason for hiding this comment

lourd commented May 13, 2022 •

edited

Loading

changeset-bot bot commented May 13, 2022 •

edited

Loading