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

Fix pretty format failing when printing invalid dates #6635

Merged
merged 7 commits into from
Aug 15, 2018

Conversation

huafu
Copy link
Contributor

@huafu huafu commented Jul 5, 2018

Summary

As suggested by @SimenB in #6631, this allow pretty format (snapshot serializer) to print invalid date objects instead of failing.

before:

expect(new Date(NaN)).toMatchSnapshot()
// will throw a RangeError: Invalid time value
//        at Date.toISOString (<anonymous>)

after:
The snapshot will contain Date { NaN } as of the date value.

Test plan

A test has been added.

@huafu
Copy link
Contributor Author

huafu commented Jul 5, 2018

Not sure if I should add this, but for GitHub to auto close the issue: this PR will close #6631

@codecov-io
Copy link

codecov-io commented Jul 5, 2018

Codecov Report

Merging #6635 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6635   +/-   ##
=======================================
  Coverage   63.57%   63.57%           
=======================================
  Files         235      235           
  Lines        8992     8992           
  Branches        3        4    +1     
=======================================
  Hits         5717     5717           
  Misses       3274     3274           
  Partials        1        1
Impacted Files Coverage Δ
packages/pretty-format/src/index.js 96.37% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c61baa...cea8b23. Read the comment docs.

@huafu
Copy link
Contributor Author

huafu commented Jul 5, 2018

All the checks were successful with the previous commit, I've just moved around something in the changelog (I did place it at the wrong position in the first place) and now Travis check is failing. Can someone re-start the job in TravisCI please? or tell me what in 8cf31da is making travis to fail :-D

@huafu
Copy link
Contributor Author

huafu commented Jul 5, 2018

...ah yeah, GitHub status saying We're investigating reports of intermittent connection failures.... Should I force push or wait for someone to restart just the travis check?

@SimenB
Copy link
Member

SimenB commented Jul 5, 2018

Don't mind Travis 🙂

@SimenB SimenB requested a review from rickhanlonii July 5, 2018 21:52
@rickhanlonii
Copy link
Member

rickhanlonii commented Jul 6, 2018

Good stuff, is there something better we can use than "invalid-date"

Some ideas:

  • Date(invalid)
  • [Date invalid]
  • Date<invalid>

@huafu
Copy link
Contributor Author

huafu commented Jul 6, 2018

@SimenB thanks for reviewing.
@rickhanlonii I chose invalid-date because I thought it was going well between true, false, null, undefined. Tho an invalid date is an object and not a primitive after all. And objects, sets, maps, ... are ClassName {...}.
So if it has to not be invalid-date, I'd rather go to something like one of those 2, to stay as close as possible to other kind of object instance values:

  1. Date { Invalid }
  2. Date { NaN }

I actually like the second one, since an invalid date's primitive value is NaN. Lemme know what I should change to and I'll do it.

@huafu
Copy link
Contributor Author

huafu commented Jul 6, 2018

...one thing tho, we have to remember that valid dates do not have Date anywhere around them:

  "date1": 2018-01-01T00:00:00.000Z
  "date2": [Date Invalid]
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": Date(invalid)
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": Date { NaN }
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": invalid-date

@rickhanlonii
Copy link
Member

My thinking is that "invalid-date" looks like a string and we pretty-print things other types as:

  • Map { ... }
  • Set { ... }
  • Arguments [ ... ]
  • DomNode [ ... ]

So it would make sense that we include the type for the invalid date:

  • Date invalid-date
  • Date { invalid }
  • Date [invalid]

Check out the way we pretty print other types and try to stay consistent. One issue with the current implementation would be the diff for a test like:

test('example', () => {
  expect(Date(NaN)).toEqual('invalid-date');
});

Would fail and have a visually indistinguishable diff

@huafu
Copy link
Contributor Author

huafu commented Jul 6, 2018

@rickhanlonii did you miss my first reply to your message? ;-)

invalid-date looks like a string

I thought that too at first, but strings are surrounded with double quotes.

Anyway, as I said in my first reply, what about Date { NaN }? I like Date { invalid } but again NaN is the primitive value of invalid dates, so why not using it as well?

So you confirm, I guess it's either:

  1. Date { invalid } or
  2. Date { NaN }

I'm ready to change it now as soon as I got your choice ;-)

@rickhanlonii
Copy link
Member

Oh shoot I missed that, good call out

I like Date { NaN } but both are good with me 🤙

@huafu
Copy link
Contributor Author

huafu commented Jul 12, 2018

Any ETA on when this will be merged @SimenB ? (or should I not worry about resolving conflicts in CHANGELOG.md anymore 🤣 ?)

@SimenB
Copy link
Member

SimenB commented Jul 12, 2018

We can take care of a merge conflict if needed when we merge, no need for you to keep updating it 🙂

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

@rickhanlonii does this LGTY? If so, we can probably merge it 🙂

@SimenB
Copy link
Member

SimenB commented Aug 15, 2018

@rickhanlonii pingeling 😀

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM!

@rickhanlonii rickhanlonii merged commit 5d65701 into jestjs:master Aug 15, 2018
@@ -146,7 +146,7 @@ function printBasicValue(
return printSymbol(val);
}
if (toStringed === '[object Date]') {
return toISOString.call(val);
return isNaN(+val) ? 'Date { NaN }' : toISOString.call(val);
Copy link
Member

@SimenB SimenB Aug 15, 2018

Choose a reason for hiding this comment

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

We could also keep around Date.prototype.toString and fall back to that one. It prints Invalid Date.

const toStringValue = toString.call(val);

return toStringValue === 'Invalid Date' ? toStringValue : toISOString.call(val);
console.log(new Date(Infinity)) // 'Invalid Date'
console.log(new Date(NaN)) // 'Invalid Date'

EDIT: Or we can do Date { Invalid }.

return toString.call(val) === 'Invalid Date'
  ? 'Date { Invalid }'
  : toISOString.call(val);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

too late :-p
anyway, I do not like testing over a "human" string. While an invalid date will always cast to the wonderful JavaScript number... not a number 🤣

Copy link
Member

Choose a reason for hiding this comment

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

Hah, too late 😀

Feel free to send a new PR with this change if you think it makes sense

@huafu huafu deleted the feature/serialize-invalid-date branch September 11, 2018 11:54
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants