fix(reducer-list): ReducerList factory now throws an error for falsy input #212

abepeterkin · 2018-02-13T22:14:02Z

BREAKING CHANGE: Anything relying on putting a falsy value into ReducerList will now
throw an error

…sy, non-string value is passed BREAKING CHANGE: Anything relying on putting a falsy, non-string value into ReducerList will now throw an error fix ViacomInc#188

coveralls · 2018-02-13T22:18:52Z

Coverage remained the same at 100.0% when pulling d31f61a on abepeterkin:bugfix/falsy-values-treated-as-reducers into 1d9801e on ViacomInc:master.

raingerber · 2018-02-13T23:18:12Z

packages/data-point/lib/reducer-types/reducer-list/factory.test.js

+    expect(() => factory.create(createReducer, [0])).toThrow()
+    expect(() => factory.create(createReducer, [undefined])).toThrow()
+    expect(() => factory.create(createReducer, ['$foo.bar', false])).toThrow()
+  })


the description implies that strings are treated differently than other types, so we should have at least one test with a string as input. having a test with a falsy string '' and another test with a string of length > 0 is even better.

why are strings treated differently though? IMO, any falsy input should be treated as an invalid reducer

raingerber · 2018-02-13T23:23:20Z

Thanks for this PR, @abepeterkin! A few quick notes:

please fill out the Checklist and what/why/how when opening a PR 😀
It looks like you didn't branch off the latest version of master - please update that branch on your fork, merge that into your feature branch, and run the tests again (there shouldn't be any changes here)

paulmolluzzo

Would like to see the tests cleaned up, and there is another comment from @raingerber that should be addressed.

paulmolluzzo · 2018-02-14T14:42:14Z

packages/data-point/lib/reducer-types/reducer-list/factory.test.js

+    expect(() => factory.create(createReducer, [0])).toThrow()
+    expect(() => factory.create(createReducer, [undefined])).toThrow()
+    expect(() => factory.create(createReducer, ['$foo.bar', false])).toThrow()
+  })


Could we please break each of these scenarios into a separate test? It makes it easier to read the tested scenarios at a glance and prints out nicer when jest is run with the --verbose flag.

For an example, you can take a look at this from #207: https://github.com/raingerber/data-point/blob/e0d3ae139baab9eb40f4591336824b34dd0794f8/packages/data-point/lib/reducer-types/reducer-helpers/utils/index.test.js#L5-L45

we should really update the contributing guidelines with the points you brought up here and on my PR #207

…hub.com/abepeterkin/data-point into bugfix/falsy-values-treated-as-reducers

abepeterkin · 2018-02-20T16:37:50Z

I split up the tests. I also added some tests for strings, and it now throws an error if you try to pass an empty string as a reducer.

raingerber · 2018-02-20T17:04:33Z

packages/data-point/lib/reducer-types/reducer-list/factory.test.js

+    expect(() => factory.create(createReducer, [undefined])).toThrow()
+  })
+
+  test('factory#throw error if reducer is zero', () => {


"factory#throw" can be replaced with "should throw" - the # is normally used to indicate a method/property, like how factory#create is used because create is a method of factory

raingerber · 2018-02-20T17:05:58Z

packages/data-point/lib/reducer-types/reducer-list/factory.js

@@ -78,7 +77,7 @@ module.exports.parse = parse
 * @param {Array} source
 * @return {reducer}
 */
-function create (createReducer, source) {
+function create (createReducer, source = []) {


why do we need a default parameter here?

There's a test case asserting that calling factory.create with only one argument should result in a ReducerList with an empty set of reducers. But this test broke when removed the calls to _.compact

We could also take this default parameter out and just change the test

Yeah let's just remove that test instead of adding the default param. Thanks

raingerber · 2018-02-20T17:22:27Z

packages/data-point/lib/reducer-types/reducer-list/factory.test.js

+
+  test('factory#throw error if reducer is false', () => {
+    expect(() => factory.create(createReducer, [false])).toThrow()
+  })


can we use toThrowErrorMatchingSnapshot instead of toThrow? that way, we can test to be sure that it's throwing an error with a useful message (the error message should be Invalid reducer type. Could not find a matching reducer type while parsing the value, but I think the current message is Cannot read property 'Symbol(reducer stub)' of undefined)

To get the correct error message, I think you need to update the isType function in reducer-stub.js.

before

function isType (source) { return source[REDUCER_STUB_SYMBOL] === true }

after

function isType (source) { return !!source && source[REDUCER_STUB_SYMBOL] === true }

can we use toThrowErrorMatchingSnapshot instead of toThrow? that way, we can test to be sure that it's throwing an error with a useful message (the error message should be Invalid reducer type. Could not find a matching reducer type while parsing the value, but I think the current message is Cannot read property 'Symbol(reducer stub)' of undefined)

👍 In fact, it'd be great to have errors snapshotted across the tests.

Great idea, I just added snapshots with the proper error messages.

acatl · 2018-02-21T14:27:40Z

closing this PR since it hasn't been active, please re-open it once you have it ready

Using toThrowErrorMatchingSnapshot() instead of toThrow(), fixed issue where passing null or undefined as a reducer would result in a weird error message ViacomInc#212

Whitespace changes in factory.test.js ViacomInc#212

Removed test case for calling factory.create with one argument ViacomInc#212

raingerber

thank you!

acatl · 2018-02-24T12:43:27Z

@paulmolluzzo if you have any time to check this PR, looks good to me

paulmolluzzo

LGTM. Thanks @abepeterkin! 🙌

fix(reducer-list): ReducerList factory now throws an error if any fal…

d3fa07d

…sy, non-string value is passed BREAKING CHANGE: Anything relying on putting a falsy, non-string value into ReducerList will now throw an error fix ViacomInc#188

raingerber reviewed Feb 13, 2018

View reviewed changes

raingerber assigned abepeterkin Feb 13, 2018

paulmolluzzo requested changes Feb 14, 2018

View reviewed changes

Abraham Peterkin and others added 5 commits February 20, 2018 11:16

fix(reducer-list): Remove another call to _.compact, split up new tests

fa7a351

Merge branch 'master' into bugfix/falsy-values-treated-as-reducers

0f2470c

fix(reducer-list): Added default array to factory.create

dc6724d

Merge branch 'master' into bugfix/falsy-values-treated-as-reducers

430e9f3

Merge branch 'bugfix/falsy-values-treated-as-reducers' of https://git…

a0e6475

…hub.com/abepeterkin/data-point into bugfix/falsy-values-treated-as-reducers

raingerber reviewed Feb 20, 2018

View reviewed changes

acatl closed this Feb 21, 2018

Abraham Peterkin added 2 commits February 22, 2018 15:45

Merge branch 'master' into bugfix/falsy-values-treated-as-reducers

ba1e5bb

fix(reducer-list):

5c35da4

Using toThrowErrorMatchingSnapshot() instead of toThrow(), fixed issue where passing null or undefined as a reducer would result in a weird error message ViacomInc#212

abepeterkin reopened this Feb 22, 2018

Abraham Peterkin added 2 commits February 22, 2018 17:08

fix(reducer-list):

b20515a

Whitespace changes in factory.test.js ViacomInc#212

fix(reducer-list):

fbf6014

Removed test case for calling factory.create with one argument ViacomInc#212

raingerber approved these changes Feb 23, 2018

View reviewed changes

raingerber changed the title ~~fix(reducer-list): ReducerList factory now throws an error if any fal…~~ fix(reducer-list): ReducerList factory now throws an error for falsy input Feb 23, 2018

acatl approved these changes Feb 24, 2018

View reviewed changes

paulmolluzzo approved these changes Feb 24, 2018

View reviewed changes

Merge branch 'master' into bugfix/falsy-values-treated-as-reducers

d31f61a

acatl merged commit 2c65794 into ViacomInc:master Feb 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(reducer-list): ReducerList factory now throws an error for falsy input #212

fix(reducer-list): ReducerList factory now throws an error for falsy input #212

abepeterkin commented Feb 13, 2018 •

edited by raingerber

coveralls commented Feb 13, 2018 •

edited

raingerber Feb 13, 2018 •

edited

raingerber commented Feb 13, 2018 •

edited

paulmolluzzo left a comment

paulmolluzzo Feb 14, 2018

raingerber Feb 15, 2018

abepeterkin commented Feb 20, 2018

raingerber Feb 20, 2018

raingerber Feb 20, 2018

abepeterkin Feb 22, 2018

abepeterkin Feb 22, 2018 •

edited

raingerber Feb 22, 2018

raingerber Feb 20, 2018 •

edited

paulmolluzzo Feb 20, 2018

abepeterkin Feb 22, 2018

acatl commented Feb 21, 2018

raingerber left a comment

acatl commented Feb 24, 2018

paulmolluzzo left a comment

fix(reducer-list): ReducerList factory now throws an error for falsy input #212

fix(reducer-list): ReducerList factory now throws an error for falsy input #212

Conversation

abepeterkin commented Feb 13, 2018 • edited by raingerber

coveralls commented Feb 13, 2018 • edited

raingerber Feb 13, 2018 • edited

Choose a reason for hiding this comment

raingerber commented Feb 13, 2018 • edited

paulmolluzzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepeterkin commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepeterkin Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raingerber Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatl commented Feb 21, 2018

raingerber left a comment

Choose a reason for hiding this comment

acatl commented Feb 24, 2018

paulmolluzzo left a comment

Choose a reason for hiding this comment

abepeterkin commented Feb 13, 2018 •

edited by raingerber

coveralls commented Feb 13, 2018 •

edited

raingerber Feb 13, 2018 •

edited

raingerber commented Feb 13, 2018 •

edited

abepeterkin Feb 22, 2018 •

edited

raingerber Feb 20, 2018 •

edited