Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose schedule() method #502

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Conversation

MillerSvt
Copy link
Contributor

@MillerSvt MillerSvt commented May 31, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@MillerSvt MillerSvt force-pushed the add-schedule branch 2 times, most recently from 2d03b1d to d7ef67f Compare May 31, 2024 08:00
@MillerSvt MillerSvt changed the title fix: expose schedule() method feat: expose schedule() method May 31, 2024
@just-jeb
Copy link
Owner

just-jeb commented Jun 4, 2024

Hey, could you please elaborate on the use case that you want to test?

@MillerSvt
Copy link
Contributor Author

MillerSvt commented Jun 5, 2024

Sure. Here is abstract example, but it is often found in our tests.

class EffectsService {
    showSuccessStateEffect$ = createEffect(() => this.actions$.pipe(
        ofType(showSuccessState),
        // ignore all events while message is visible
        exhaustMap(() => merge(
            // show success message for three seconds
            of(true),
            timer(timeout).pipe(map(() => false)),
        ),
    ));
}

We need to test these cases. First case is simple for test:

actions$ = new ReplaySubject(1);
actions$.next(showSuccessState());

const expected = cold('t-----f', {
    t: true,
    f: false,
});

expect(effects.showSuccessStateEffect$).toBeObservable(expected);

But how can we test whether events are being ignored?

actions$ = new ReplaySubject(1);
actions$.next(showSuccessState());

cold('---a|').subscribe(() => actions$.next(showSuccessState()));

const expected = cold('t-----f------', {
    t: true,
    f: false,
});

expect(effects.showSuccessStateEffect$).toBeObservable(expected);

But as for me, it looks ugly. It is better to use schedule() function:

actions$ = new Subject();

schedule(() => actions$.next(showSuccessState()), 2);
schedule(() => actions$.next(showSuccessState()), 4);
schedule(() => actions$.next(showSuccessState()), 11);

const expected = cold('-t-----f--t-----f---', {
    t: true,
    f: false,
});

expect(effects.showSuccessStateEffect$).toBeObservable(expected);

As I mentioned earlier, this is a hypothetical scenario. In our tests, we mock the actions$ using TestBed in the beforeEach block. Therefore, we cannot use the cold() function for actions$, as it may vary depending on the specific test case. And our effects are more complex, so we need to perform side effects more often.

@just-jeb just-jeb merged commit f7725f7 into just-jeb:master Jul 4, 2024
1 check passed
@just-jeb
Copy link
Owner

just-jeb commented Jul 4, 2024

Both your PRs have been released in 3.1.0.

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.

2 participants