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(timestamps): findOneAndUpdate creates subdocs with timestamps in reverse order #12484
Conversation
…reverse order closes Automattic#12475
makes sense |
}, { upsert: true, new: true, runValidators: true }); | ||
|
||
for (const address of newItem.addresses) { | ||
const expected = `{"location":"${address.location}","_id":"${address._id}","createdAt":"${address.createdAt.toISOString()}","updatedAt":"${address.updatedAt.toISOString()}"}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it help to make this multiline? Just for readability?
@@ -1026,6 +1026,30 @@ describe('timestamps', function() { | |||
assert.equal(doc.sections.length, 1); | |||
assert.equal(doc.sections[0].title, 'h1'); | |||
}); | |||
|
|||
it('findOneAndUpdate creates subdocuments with timestamps in reverse order', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the test cases listed here include the Github issue (e.g. gh-12475).
Also, wouldn't we describe this as:
findOneAndUpdate creates subdocuments with timestamps in the expected order
At least, that's how I'm reading the expected
variable.
Thank you @lpizzinidev for having a fix for this. Appreciate it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. I'll make a couple of quick fixes after merging but nothing that should block this.
|
||
for (const address of newItem.addresses) { | ||
const expected = `{"location":"${address.location}","_id":"${address._id}","createdAt":"${address.createdAt.toISOString()}","updatedAt":"${address.updatedAt.toISOString()}"}`; | ||
assert.equal(JSON.stringify(address), expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't recommend testing the JSON.stringify()
output, because that is typically not very readable. I would rather this test that Object.keys(address.toObject())
has 'createdAt' before 'updatedAt'
closes #12475