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 Test Coverage for `DS.NumberTransform` #2509

Merged
merged 1 commit into from Nov 25, 2014

Conversation

Projects
None yet
5 participants
@seanpdoyle
Contributor

seanpdoyle commented Nov 25, 2014

Transforms are not tested at the unit level. Adding tests at the
unit granularity allows for regression testing if the implementation covered
by the acceptance level tests should change.

equal(transform.deserialize(null), null);
equal(transform.deserialize(undefined), null);
equal(transform.serialize("1.1"), 1.1);

This comment has been minimized.

@sly7-7

sly7-7 Nov 25, 2014

Contributor

Bad copy paste I think ;)

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor

Yup, fixed 🙈

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:add-tests-number-transform branch from 26f5136 to f5a03bd Nov 25, 2014

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Nov 25, 2014

It's probably worth noting that I've kept this focussed on a single set of tests to validate whether or not PRs like this are considered valuable.

If this looks good, would it be merging in by itself, with subsequent PRs for tests for each transformer, or should I continue working on the other Transforms within this PR (or possibly within the same commit)

@sly7-7

This comment has been minimized.

Contributor

sly7-7 commented Nov 25, 2014

I would say this is a good idea, thanks for that. Probably a single commit with every tests in this PR is the good way. cc @igorT

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:add-tests-number-transform branch from f5a03bd to 46a27c3 Nov 25, 2014

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Nov 25, 2014

@sly7-7 This is ready for another look.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:add-tests-number-transform branch from 46a27c3 to 47ed921 Nov 25, 2014

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Nov 25, 2014

👍

equal(transform.serialize(null), null);
equal(transform.serialize(undefined), null);
equal(transform.serialize("1.1"), 1.1);

This comment has been minimized.

@igorT

igorT Nov 25, 2014

Member

add a number here?

This comment has been minimized.

@fivetanley

fivetanley Nov 25, 2014

Member

a "new Number(1.1)" and NaN would be good to test as well.

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor

@fivetanley what would the expected value of serialize(NaN) be?

This comment has been minimized.

@igorT

igorT Nov 25, 2014

Member

NaN?

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor

Does the same go for deserialize? Is NaN a valid JSON value, or would it have to be wrapped in a string?

This comment has been minimized.

@igorT

igorT Nov 25, 2014

Member

NaN is not a valid JSON value, not sure what we should do. What does it do now?

This comment has been minimized.

@stefanpenner

stefanpenner Nov 25, 2014

Member

NaN isn't valid json, also for reference neither is Infinity

JSON.stringify({a: NaN, b: Infinity}); // => "{"a":null,"b":null}"

should we warn/error/ignore?

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor
equal(transform.serialize(NaN), NaN);

is failing. Probably because:

Ember.isEqual(NaN, NaN) // => false

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor

Should the transform (de)serialize(NaN) === null?

This comment has been minimized.

@stefanpenner

stefanpenner Nov 25, 2014

Member

yes null for both NaN, Infinity and -Infinity

@igorT

This comment has been minimized.

Member

igorT commented Nov 25, 2014

Looks great!

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:add-tests-number-transform branch from 47ed921 to ba5ce65 Nov 25, 2014

Add Test Coverage for `ember-data/transforms`
Transforms are not tested at the `unit` level. Adding tests at the
`unit` granularity allows for regression testing if the
implementation covered by the `acceptance` level tests should change.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:add-tests-number-transform branch from ba5ce65 to c880dca Nov 25, 2014

equal(transform.deserialize(1.1), 1.1);
equal(transform.deserialize(new Number(1.1)), 1.1);
ok(isNaN(transform.deserialize(NaN)));

This comment has been minimized.

@seanpdoyle

seanpdoyle Nov 25, 2014

Contributor

@stefanpenner @igorT This covers the current implementation, but I'm not too keen on the approach.

If I were to swap in:

equal(transform.deserialize(NaN), null);

I would have to change the implementation

This comment has been minimized.

@stefanpenner

stefanpenner Nov 25, 2014

Member

I am fine with no caring about NaN for now, but it would be nice to have.

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Nov 25, 2014

@stefanpenner I'm fine with supporting serialize({NaN,Infinity,-Infinity}) === null in a separate PR

seanpdoyle added a commit to seanpdoyle/data that referenced this pull request Nov 25, 2014

[BUG] Transform `{NaN,Infitity,-Infinity}` to null
emberjs#2509 (comment)

Serializing and Deserializing invalid numeric `JSON` (`NaN`,
`Infinity`, `-Infinity`) should result in `null`.
@seanpdoyle

This comment has been minimized.

stefanpenner added a commit that referenced this pull request Nov 25, 2014

Merge pull request #2509 from seanpdoyle/add-tests-number-transform
Add Test Coverage for `DS.NumberTransform`

@stefanpenner stefanpenner merged commit 0189f92 into emberjs:master Nov 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Nov 25, 2014

@seanpdoyle awesome, thank you kindly sir!

seanpdoyle added a commit to seanpdoyle/data that referenced this pull request Nov 25, 2014

[BUG] Transform `{NaN,Infitity,-Infinity}` to null
emberjs#2509 (comment)

Serializing and Deserializing invalid numeric `JSON` (`NaN`,
`Infinity`, `-Infinity`) should result in `null`.

@seanpdoyle seanpdoyle deleted the seanpdoyle:add-tests-number-transform branch Nov 25, 2014

seanpdoyle added a commit to seanpdoyle/data that referenced this pull request Nov 25, 2014

[BUG] Transform `{NaN,Infitity,-Infinity}` to null
emberjs#2509 (comment)

Serializing and Deserializing invalid numeric `JSON` (`NaN`,
`Infinity`, `-Infinity`) should result in `null`.

fivetanley added a commit that referenced this pull request Nov 27, 2014

[BUG] Transform `{NaN,Infitity,-Infinity}` to null
#2509 (comment)

Serializing and Deserializing invalid numeric `JSON` (`NaN`,
`Infinity`, `-Infinity`) should result in `null`.

igorT added a commit that referenced this pull request Nov 28, 2014

[BUG] Transform `{NaN,Infitity,-Infinity}` to null
#2509 (comment)

Serializing and Deserializing invalid numeric `JSON` (`NaN`,
`Infinity`, `-Infinity`) should result in `null`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment