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

Set created_at for jobs based on table row #10

Merged
merged 3 commits into from
May 17, 2018
Merged

Set created_at for jobs based on table row #10

merged 3 commits into from
May 17, 2018

Conversation

tjwp
Copy link
Contributor

@tjwp tjwp commented May 17, 2018

What did we change?

Pass in the created_at value from the jobs table when enqueueing Sidekiq jobs.

Remove unnecessary code for manually publishing a single job.

Why are we doing this?

Sidekiq allows this value be passed in or defaults it to the current time if unset.

How was it tested?

  • Specs
  • Locally
  • Staging

@tjwp tjwp requested a review from zoso10 May 17, 2018 14:55
Copy link
Contributor

@zoso10 zoso10 left a comment

Choose a reason for hiding this comment

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

why weren't we populating this before?

@@ -157,6 +100,7 @@
run_at: nil,
queue: nil,
wrapped: nil,
created_at: be_within(10**-6).of(job.created_at),
Copy link
Contributor

Choose a reason for hiding this comment

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

heh i guess we dont have any time helpers do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I'm aware of. Are there common ones you'd suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

timecop for freezing time, but im totally cool with this in lieu of adding another dev dependency

Copy link
Contributor Author

@tjwp tjwp May 17, 2018

Choose a reason for hiding this comment

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

Ah, in this case it's not a freezing issue, it's a rounding/precision issue for the same value. I've run into this before that you'll get slightly more precision on Linux vs OSX, so things that match exactly locally will differ in CI.

Update: specifically a precision issue after database round-tripping

Copy link
Contributor

@zoso10 zoso10 May 17, 2018

Choose a reason for hiding this comment

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

oh sorry! wasnt looking close enough. we have a custom match_time matcher and with_database_precision helper in ez-rails takes care of precision issues of time objects in mem and time from the db

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave this as-is for now, but maybe eventually we extract that to ezcater_matchers.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed! getting off track now but we should audit all the custom matchers in ez-rails at some point and decide what to move to ezcater_matchers

@tjwp
Copy link
Contributor Author

tjwp commented May 17, 2018

I just didn't include it in the first pass since it wasn't required.

@tjwp tjwp merged commit 6118659 into master May 17, 2018
@tjwp tjwp deleted the tjwp/create-at branch May 17, 2018 15:39
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.

None yet

2 participants