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

Marble testing doc additions #4522

Merged
merged 13 commits into from Mar 7, 2019

Conversation

whiteinge
Copy link
Contributor

Description:

Additions and adjustments to the marble testing documentation. These additions emphasize the async helper functions, and also make the subscriptionMarbles parameter more explicit.

Related issue (if exists):

Closes #4521.

@coveralls
Copy link

coveralls commented Jan 31, 2019

Pull Request Test Coverage Report for Build 8193

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.962%

Totals Coverage Status
Change from base Build 8187: 0.0%
Covered Lines: 5777
Relevant Lines: 5958

💛 - Coveralls

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

Some minor nits.

- `hot(marbleDiagram: string, values?: object, error?: any)` - creates a ["hot" observable](https://medium.com/@benlesh/hot-vs-cold-observables-f8094ed53339) (like a subject) that will behave as though it's already "running" when the test begins. An interesting difference is that `hot` marbles allow a `^` character to signal where the "zero frame" is. That is the point at which the subscription to observables being tested begins.
- `cold(marbleDiagram: string, values?: object, error?: any)` - creates a ["cold" observable](https://medium.com/@benlesh/hot-vs-cold-observables-f8094ed53339) whose subscription starts when the test begins.
- `expectObservable(actual: Observable<T>).toBe(marbleDiagram: string, values?: object, error?: any)` - schedules an assertion for when the TestScheduler flushes. Give `subscriptionMarbles` as parameter to change the schedule of subscription and unsubscription. If you don't provide the `subscriptionMarbles` parameter it will subscribe at the beginning and never unsubscribe. Read below about subscription marble diagram.
- `expectObservable(actual: Observable<T>, subscriptionMarbles: string).toBe(marbleDiagram: string, values?: object, error?: any)` - schedules an assertion for when the TestScheduler flushes. Give `subscriptionMarbles` as parameter to change the schedule of subscription and unsubscription. If you don't provide the `subscriptionMarbles` parameter it will subscribe at the beginning and never unsubscribe. Read below about subscription marble diagram.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameter is optional: subscriptionMarbles?: string

expectObservable(repeatingStream$, unsub).toBe(results);
});
})
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make this a complete example? results is not defined.

Copy link
Member

Choose a reason for hiding this comment

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

@whiteinge ... can you also add a example of using the subscription marbles to delay the subscription?

For example, given a hot source, you can subscribe to it more than once and verify the results:

testScheduler.run(({ hot, expectObservable }) => {
  const source = hot('--a--a--a--a--a--a--a--');
  const sub1 = '      --^-----------!';
  const sub2 = '      ---------^--------!';
  const expect1 = '   --a--a--a--a--';
  const expect2 = '   -----------a--a--a-';
  expectObservable(source, sub1).toBe(expect1);
  expectObservable(source, sub2).toBe(expect2);
});

Why: This is a complete example, as requested, and also more plain to
demonstrate just the one param.
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

@rxjs-bot
Copy link

rxjs-bot commented Feb 1, 2019

Warnings
⚠️

commit message does not follows conventional change log (1)

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

Generated by 🚫 dangerJS

@whiteinge
Copy link
Contributor Author

Will the change log bot ignore those two fixup commits? Or should I rebase them into their respective initial commits?

@cartant
Copy link
Collaborator

cartant commented Feb 1, 2019

All of the commits in here are so closely related that I would expect the person merging to squash them, so I wouldn't worry about it.

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Some minor nits, and a request for one more example (I provided one, if that helps).

doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
doc/marble-testing.md Outdated Show resolved Hide resolved
expectObservable(repeatingStream$, unsub).toBe(results);
});
})
```
Copy link
Member

Choose a reason for hiding this comment

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

@whiteinge ... can you also add a example of using the subscription marbles to delay the subscription?

For example, given a hot source, you can subscribe to it more than once and verify the results:

testScheduler.run(({ hot, expectObservable }) => {
  const source = hot('--a--a--a--a--a--a--a--');
  const sub1 = '      --^-----------!';
  const sub2 = '      ---------^--------!';
  const expect1 = '   --a--a--a--a--';
  const expect2 = '   -----------a--a--a-';
  expectObservable(source, sub1).toBe(expect1);
  expectObservable(source, sub2).toBe(expect2);
});

Copy link
Member

@niklas-wortmann niklas-wortmann left a comment

Choose a reason for hiding this comment

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

Could you also move this document to docs_app/content/guide (not 100% sure about the path) and make a minor adjustment in the navigation.json file. That this change will directly be in the new docs! If you need any kind of help with this, just let me know. I would love to have this in the new docs, so thank you very much!

@whiteinge
Copy link
Contributor Author

@jwo719 done. Thanks for the pointer. I didn't realize there was a spot for those docs in the guide and that's a much better (and nicer looking) place.

Copy link
Member

@niklas-wortmann niklas-wortmann left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Fantastic addition! Thank you.

@benlesh benlesh merged commit ad8f7b3 into ReactiveX:master Mar 7, 2019
@whiteinge whiteinge deleted the marble-testing-doc-additions branch March 7, 2019 18:03
@lock lock bot locked as resolved and limited conversation to collaborators Apr 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants