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

Add next handler for Observables' tests #608

Merged
merged 6 commits into from
Mar 1, 2021

Conversation

dingyuchen
Copy link
Contributor

Summary

This fixes #607, where not supplying the next handler lets the test pass automatically when the desired behavior is for an operator to throw an error.

Proposed Commit Message

Tests: Some Subscribers are missing its next handler

Let's add next handlers where appropriate so that tests
fail when there is undesired behavior.

@dingyuchen dingyuchen marked this pull request as draft February 20, 2021 05:41
@dingyuchen
Copy link
Contributor Author

Before I make this PR ready for review @ptvrajsk could you help me take a look at whether I caught all the instances of missing next handlers?

I did a search on all files that matches expect(err) and I mainly only found tests in session.model.spec, of which most testcases are fixed. If you are aware of any other places there might be bugs do let me know! Thanks

@ptvrajsk
Copy link
Contributor

Sure @dingyuchen I'll look through them 👍

Copy link
Contributor

@ptvrajsk ptvrajsk left a comment

Choose a reason for hiding this comment

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

I checked through my IDE as well and I don't think you missed out on any. Nice job! Just some minor parts to discuss.

tests/app/core/models/session-model.spec.ts Outdated Show resolved Hide resolved
@dingyuchen dingyuchen marked this pull request as ready for review February 28, 2021 05:02
@dingyuchen dingyuchen requested a review from a team February 28, 2021 11:51
Copy link
Contributor

@ptvrajsk ptvrajsk left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dingyuchen dingyuchen requested a review from anubh-v March 1, 2021 04:35
@anubh-v anubh-v changed the title Adding next handler for pipe operator tests Add next handler for Observables' tests Mar 1, 2021
@anubh-v anubh-v merged commit 91a350f into CATcher-org:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supply next: condition in TestCases that expect an error
3 participants