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

Store epoch to correct entry, fix uninitialized variable #40

Closed
wants to merge 2 commits into from

Conversation

JonOfUs
Copy link
Collaborator

@JonOfUs JonOfUs commented Oct 13, 2021

Found a bug in the migration: the UPDATE SQL query updates all entries where timestamp_epoch is 0. On my setup (sqlite) this lead to identical timestamp_epoch values in each updated entry. Selecting the entries by their id's fixes this.

Additionally, this fixes #39 by initializing $result.

@thrillfall
Copy link
Owner

Thanks for fixing this. But can you please add a test that verifies the correct value migration? Should have done it myself before, but hey 🤦

@JonOfUs
Copy link
Collaborator Author

JonOfUs commented Oct 13, 2021

Do you mean a unit test for developing purposes or a test inside the migration function itself?

@thrillfall
Copy link
Owner

Do you mean an unit test for developing purposes or a test inside the migration function itself?

The first please.
What would happen if the test failed when you test within the migration function? I dont get it 😄

@JonOfUs
Copy link
Collaborator Author

JonOfUs commented Oct 13, 2021

I probably don't have time until tomorrow, so if you have time, you could add the test too. Since we already have a test for the migration feature that only tests with one EpisodeAction (so it didn't find the bug) this change shouldn't take too long.

I just wasn't sure what kind of test you mean. Ensuring that all carried out migrations went right in running instances could be useful because now probably many timestamps are incorrect (prob. ignorable problem, because it's a constantly changing value).

@thrillfall
Copy link
Owner

thrillfall commented Oct 13, 2021

I extend the test.
Lesson learned (yet again): never test with only one item. 🤦

@thrillfall
Copy link
Owner

pushed you all commit to main

@thrillfall thrillfall closed this Oct 13, 2021
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.

Error on installing 3.0.0 from app store
2 participants