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.

Sign up for GitHub

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

Jump to bottom

test(deferObservable): add type definition test #1479

Closed
wants to merge 1 commit into from
Closed

test(deferObservable): add type definition test #1479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions spec/observables/defer-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as Rx from '../../dist/cjs/Rx';
declare const {hot, expectObservable, expectSubscriptions};
declare const {hot, expectObservable, expectSubscriptions, type};

const Observable = Rx.Observable;

Expand Down Expand Up @@ -40,9 +40,9 @@ describe('Observable.defer', () => {

it('should create an observable when factory throws', () => {
//type definition need to be updated
const e1 = Observable.defer(<any>(() => {
const e1 = Observable.defer(() => {
throw 'error';
}));
});
const expected = '#';

expectObservable(e1).toBe(expected);
Expand Down Expand Up @@ -72,4 +72,16 @@ describe('Observable.defer', () => {
expectObservable(e1, unsub).toBe(expected);
expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

type(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This might be the wrong time to point this out... but I have NO IDEA what these tests do. I find them almost unreadable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there's wrong time, or even it'd be great time since we don't have much test for this yet. :)

Basically this test is ensuring type definition is 'works' as it's actual functionality does, for instance in case of this one defer, type should support cases

  • when factory function returns observable<T>, subscribe should carry over type T so you can access its properties without type errors
  • factory function can throw without return anything, so defer should accept function throws but does not return

both of declaration in line 82 / 83 illustrates those usecases, and since it's compile time it is being verified when test case is being compiled. If those are not working as intended, compiler will throw out messages of expected type vs. provided one.

I also agree it could be possibly confusing to consume this kind of test (small one would be ok, but case like this? https:/ReactiveX/RxJS/pull/1189/files) and it'd be great if there's suggestion how to improve this or requirement should be added for better visibility. Since there is barely no test yet, changes can be easily introduced.

Copy link
Member

Choose a reason for hiding this comment

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

it seems like it would be better just to work that into the TypeScript of our actual tests. Since those will cause the compilation of the spec file themselves to fail and thus cause the tests to fail. Do we really need an additional test that doesn't seem to do anything once it's transpiled?

Copy link
Member Author

Choose a reason for hiding this comment

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

feasible opinion. In most cases test cases already consumes type definitions, so probably won't need to be covered once again via explicit. I remember few cases that test case does not include all of possible type definition cases though - start with opt out and only add necessary cases once it's being noticed maybe better way to go?

Copy link
Member Author

Choose a reason for hiding this comment

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

overloaded signature like combinelatest (#1189) might be few edge cases need these kind of test cases.

const typeValue = {
val: 3,
str: 'str'
};

Observable.defer(() => Observable.of(typeValue)).subscribe(x => { x.str.toString(); });
Observable.defer(() => {
throw 'error';
}).subscribe();
});
});
6 changes: 3 additions & 3 deletions src/observable/DeferObservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ export class DeferObservable<T> extends Observable<T> {
* @name defer
* @owner Observable
*/
static create<T>(observableFactory: () => Observable<T>): Observable<T> {
static create<T>(observableFactory: () => Observable<T> | void): Observable<T> {
return new DeferObservable(observableFactory);
}

constructor(private observableFactory: () => Observable<T>) {
constructor(private observableFactory: () => Observable<T> | void) {
super();
}

Expand All @@ -28,7 +28,7 @@ export class DeferObservable<T> extends Observable<T> {
if (result === errorObject) {
subscriber.error(errorObject.e);
} else {
result.subscribe(subscriber);
(<Observable<T>>result).subscribe(subscriber);
}
}
}