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

Run exception tests for all ActiveJob adapters and fix issue with individual counters and Resque #34890

Merged
merged 2 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@rosa
Copy link
Member

commented Jan 7, 2019

Summary

ActiveJob exception tests were being run multiple times, one per adapter, but using always the test adapter. This made a bug that happened only for resque to go undetected. This PR tackles both issues.

More details

ExceptionsTest for ActiveJob currently extends ActiveJob::TestCase instead of ActiveSupport::TestCase as the other tests. This has the consequence that the test adapter provided for tests was being used always, in all executions where supposedly different adapters were being used. This pull request rewrites the tests so that they can be run without extending ActiveJob::TestCase.

These tests cover the new individual execution counters for exceptions (#34352). We found an issue with these in our app that uses resque: the individual counters are stored as Hash.new(0), which after being serialized, stored in Redis, and deserialized, becomes a regular hash without a default value. The tests were passing because they weren't being executed with resque at all. This pull request fixes that small bug as well.

@rails-bot rails-bot bot added the activejob label Jan 7, 2019

@rosa rosa force-pushed the rosa:test-exceptions-with-all-adapters branch 2 times, most recently from 890cd4a to 6392cf4 Jan 7, 2019

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Test are still broken, can you take a look?

rosa added some commits Jan 7, 2019

Rewrite ActiveJob exception tests so it runs with the real adapters
Previously, by extending ActiveJob::TestCase, the test adapter provided
for tests was being used always, in all executions where supposedly
different adapters were being used. As a consequence, some bugs visible
only for some adapters might have gone undetected. This commit changes
that, skipping queue adapters for which we can't test scheduling jobs
with a delay.
Ensure 0 is always the default for the individual exception counters …
…in ActiveJob

Some adapters like Resque that use Redis, convert the Ruby hash with a
default value, Hash.new(0), into a regular hash without a default value
after serializing, storing and deserializing. This raises an error when
we try to access a missing exception key. A simple solution is not to
rely on the hash's default value, and provide a default as alternative
when accessing it instead.

@rosa rosa force-pushed the rosa:test-exceptions-with-all-adapters branch from 6392cf4 to acbbd4a Jan 8, 2019

@rosa

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

@rafaelfranca they should be fine now, sorry about that! Since we weren't running these tests for the real adapters, some of them didn't support testing with scheduled jobs (i.e., enqueued with enqueued_at, that is always used when retrying jobs). I've skipped the tests for these, in the same way we were already skipping them for the :inline adapter (even though they weren't run for it).

@rafaelfranca rafaelfranca merged commit 4837531 into rails:master Jan 8, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rosa rosa deleted the rosa:test-exceptions-with-all-adapters branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.