test(scan-spec): Change use of .reduce to .scan in scan-spec.ts#2902
test(scan-spec): Change use of .reduce to .scan in scan-spec.ts#2902benlesh merged 1 commit intoReactiveX:masterfrom
Conversation
spec/operators/scan-spec.ts
Outdated
| @@ -231,14 +231,14 @@ describe('Observable.prototype.scan', () => { | |||
| it('should accept array typed reducers', () => { | |||
There was a problem hiding this comment.
Even test name seems we just copied test but haven't updated? 😱
There was a problem hiding this comment.
The test name still seemed to make sense to me. I'm not entirely certain the tests are actually fully correct. I'll leave references in the actual code.
There was a problem hiding this comment.
Should I just remove the word reducers from the test name?
There was a problem hiding this comment.
yeah what I'm saying is these test seems like just copied from reduce and we haven't updated at all. Not saying test case name should be updated as well.
Generated by 🚫 dangerJS |
-+32.9KB-red.svg?style=flat-square){kind=icon}
-+2.3KB-red.svg?style=flat-square){kind=icon}
| @@ -249,7 +249,7 @@ describe('Observable.prototype.scan', () => { | |||
| it('should accept R typed reducers', () => { | |||
There was a problem hiding this comment.
This test dosen't seem right. Shouldn't the actual call to .scan be something like this?
a.scan<{ a: number, b: string }, { a?: number; b?: string }>((acc, value) => { ...There was a problem hiding this comment.
isn't it for T only overload testing?
scan<T>(this: Observable<T>, accumulator: (acc: T, value: T, index: number) => T, seed?: T)
There was a problem hiding this comment.
@kwonoj Well, the test is called should accept R typed reducers. I'm assumed that meant we're explicitly testing the case where we have two different types for acc and the source. I'm guessing that based on the other tests in the file, i.e, there's another one for T typed reducers and `Array typed reducers'
There was a problem hiding this comment.
I think we can't just trust test name and test cases here at all, since from first it was testing reduce anyway 😅 I was just guessing by usage of code here.
There was a problem hiding this comment.
Because of the way the overloads and the tests are set-up, in the actual test, both acc and value are T. Which means acc isn't really inheriting what R is supposed to be.
There was a problem hiding this comment.
Maybe we should leave this as-is for now, and i'll propose some changes to these tests in #2897 after we get this in ?
spec/operators/scan-spec.ts
Outdated
| @@ -231,14 +231,14 @@ describe('Observable.prototype.scan', () => { | |||
| it('should accept array typed reducers', () => { | |||
There was a problem hiding this comment.
Should I just remove the word reducers from the test name?
spec/operators/scan-spec.ts
Outdated
| @@ -231,14 +231,14 @@ describe('Observable.prototype.scan', () => { | |||
| it('should accept array typed reducers', () => { | |||
spec/operators/scan-spec.ts
Outdated
| let a: Rx.Observable<{ a: number; b: string }>; | ||
| a.reduce<{ a?: number; b?: string }>((acc, value) => { | ||
| a.scan<{ a?: number; b?: string }>((acc, value) => { | ||
| value.a = acc.a; |
There was a problem hiding this comment.
Actually, isn't this supposed to be acc.a = value.a? I'll fix this now.
hsubra89
force-pushed
the
fix-scan-tests
branch
from
c86264c to
8958fe9
Compare
Change the use of `.reduce` from scan-spec to correctly use `.scan` instead. None
hsubra89
force-pushed
the
fix-scan-tests
branch
from
8958fe9 to
923195f
Compare
1 similar comment
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
5 participants
Test the .scan operator from
scan-spec.tsinstead of.reduce