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

Send arguments with exception in active job plugin #505

Closed
wants to merge 2 commits into from

Conversation

PlugIN73
Copy link
Contributor

@PlugIN73 PlugIN73 commented Aug 5, 2016

Hi! We are using active job plugin and we have troubles to understand what's wrong in our code without arguments of this job.
In Active Job, arguments it's method like job_id that you are using

@jondeandres
Copy link
Contributor

@PlugIN73 Can you take a look at the tests? They are failing cause your change.

This looks good to me, thanks for the PR!

@PlugIN73 PlugIN73 force-pushed the patch-1 branch 3 times, most recently from 5d0da06 to 75fdd9a Compare August 5, 2016 15:18
@PlugIN73
Copy link
Contributor Author

PlugIN73 commented Aug 5, 2016

@jondeandres oh) I fixed tests. But this build was failed without my changes https://travis-ci.org/rollbar/rollbar-gem/jobs/150082517

@jondeandres
Copy link
Contributor

@PlugIN73 thanks for adding/fixing those tests! For the build is failing, we have a commit in another PR, ad694bb, which is still not merged. Would you mind do a git fetch and cherry-pick that commit? That should fix that build and your PR will be green 😄.

Thanks!

@PlugIN73
Copy link
Contributor Author

PlugIN73 commented Aug 9, 2016

@jondeandres done :) all checks have passed. can you merge it? :)

@PlugIN73
Copy link
Contributor Author

@jondeandres ping :)

@@ -9,6 +9,11 @@ class TestJob
include Rollbar::ActiveJob

attr_reader :job_id
attr_accessor :arguments

def initialize(*arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

@PlugIN73 I guess with this you are giving the same behavior than for a real ActiveJob job. I have a doubt, I think arguments will return the arguments for the perform method, no?

Can you make this test more close to a real scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how ActiveJob does it

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is confusing, ActiveJob kinda does it this way, peform_now basically calls perform(*arguments) where arguments is set in init. So to get these to match, the arguments should match up with what is passed to perform because all of this is fake.

Copy link
Contributor

@rokob rokob left a comment

Choose a reason for hiding this comment

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

Needs a rebase and some test work.

@rivkahstandig3636 rivkahstandig3636 added this to the v2.16.0 milestone Dec 27, 2017
@rivkahstandig3636 rivkahstandig3636 modified the milestones: v2.16.0, v2.16.1 Apr 18, 2018
@ArturMoczulski ArturMoczulski modified the milestones: v2.16.1, v2.16.2 May 30, 2018
@vtm9
Copy link

vtm9 commented Jun 2, 2018

@PlugIN73 , maybe you resolve conflicts?

@rivkahstandig3636 rivkahstandig3636 modified the milestones: v2.16.2, v2.16.3 Jun 17, 2018
@ArturMoczulski ArturMoczulski modified the milestones: v2.16.3, v2.16.4 Jul 3, 2018
@ArturMoczulski ArturMoczulski modified the milestones: v2.16.4, v2.16.5 Jul 30, 2018
@jessewgibbs
Copy link
Contributor

@ArturMoczulski what do you want to do w/ this PR? It's over 2 years old now...

@rivkahstandig3636
Copy link
Contributor

@ArturMoczulski @jessewgibbs I'm just creating an issue for this and will link it here and close this PR.

@ArturMoczulski
Copy link
Contributor

Going through it now

@ArturMoczulski
Copy link
Contributor

This has now been merged in PR #772

@ArturMoczulski ArturMoczulski added this to the v2.18.0 milestone Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants