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

failed test for null datetime field #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matteorebeschi
Copy link

This is the failing test for the datetime field as discussed in #47

@jeremyharris
Copy link
Collaborator

jeremyharris commented Apr 7, 2022

Great, thanks! Test looks good. Now that we have a test, if you like you can put your first fix in here. I've got some ideas on how to address it and improve the code you posted, if you don't mind me tweaking it.

@matteorebeschi
Copy link
Author

I've tried changing the convertFieldsToType to what I mentioned in the error thread, but the tests keep failing, so I'm not even sure that that's a good starting point anymore.
But feel free to go ahead and tweak it if you have any ideas!

@jeremyharris
Copy link
Collaborator

Hey @matteorebeschi, I was able to look at this. I looked at your tests and I think you had the fixture set up incorrectly.

I reworked the fixture and added tests for both null support and DateTime. The tests still pass, so it can successfully version null values and DateTimes. It never serializes the data in the way the original issues shows.

So far I can't replicate the problem

  1. Are you able to tweak testDateTimeSupport or copy/paste it and recreate your scenario, that is, saving some datetime/null data and it failing to return from ->version()?
  2. Are you using a custom database Type for your datetimes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants