-
Notifications
You must be signed in to change notification settings - Fork 39
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
Feature/background fetch super admin #100
Feature/background fetch super admin #100
Conversation
features/admin.feature
Outdated
@@ -226,7 +226,8 @@ Feature: Administration | |||
Then I should see "search.gov/oops" | |||
And I follow "Fetch" | |||
And I wait for ajax | |||
Then I should see "404" | |||
Then I should see "Your URL has been added to the fetching queue" | |||
And I follow "Close" within "div.info-message.message" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing , remember to keep cucumber features as 'human-readable' as possible. For within
steps, you'll need to define one a 'human-readable' page area. See https://github.com/GSA/search-gov/blob/master/features/step_definitions/within_steps.rb
In this case, you might say something like within the first URLs row
. I suspect that div.info-message.message
isn't the best CSS id for that; let me know if you need help finding a better identifier.
require 'spec_helper' | ||
|
||
describe SearchgovUrlFetcherJob do | ||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(args[:searchgov_url]) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you aren't using keyword arguments for this job, you can just say:
subject(:perform) { SearchgovUrlFetcherJob.perform_now(searchgov_url) }
...and remove the args
variable.
{ searchgov_url: searchgov_url } | ||
end | ||
|
||
it_behaves_like 'a searchgov job' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌟 👍
|
||
describe SearchgovUrlFetcherJob do | ||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(args[:searchgov_url]) } | ||
let!(:searchgov_url) { SearchgovUrl.create(url: 'https://agency.gov/') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure you need the bang method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I do, as the tests don't seem to pass if I don't have it. Maybe there's a better way of organizing it?
it 'fetches a searchgov_url' do | ||
# byebug | ||
perform | ||
expect(searchgov_url.last_crawl_status).to_not be(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need the record to be fetched in our test, nor do we need the test to know anything about what fetch
does (such as changing the last_crawl_status
). A cleaner (and faster) way to test this would be to use a mock, i.e.:
expect(searchgov_url).to receive(:fetch)
perform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this example to what you've suggested. I'm not sure if you meant to scrap it completely and were just giving me an example of how it should be done were we trying to test it, so if you meant for me to get rid of it I can do that real quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change you made is what I was looking for 👍
@@ -1,7 +1,7 @@ | |||
require 'spec_helper' | |||
|
|||
describe SearchgovUrlFetcherJob do | |||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(args[:searchgov_url]) } | |||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(searchgov_url) } | |||
let!(:searchgov_domain) { SearchgovDomain.create(domain: 'agency.gov', status: '200') } | |||
let(:searchgov_url) { SearchgovUrl.create(url: 'https://agency.gov/') } | |||
let(:args) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my earlier comment, you can remove the args
variable, which you are no longer using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the args
variable is necessary for the shared example, it_behaves_like 'a searchgov job'
, to work correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you are correct. I was thinking you could just pass the searchgov_url
record as the argument, rather than a hash, because you are not using keyword arguments.
However...let's change that. I am not always consistent about using keyword args, but I do like how explicit they are about complaining when you are missing an argument. Using keyword args would also be consistent with the other job arguments:
$ gg 'def perform' app/jobs/
app/jobs/searchgov_domain_indexer_job.rb: def perform(searchgov_domain:, delay:)
app/jobs/searchgov_domain_preparer_job.rb: def perform(searchgov_domain:)
app/jobs/sitemap_indexer_job.rb: def perform(searchgov_domain:)
So, instead of:
def perform(searchgov_url)
You'd use:
def perform(searchgov_url:)
And the way to initiate that job would be:
SearchgovUrlFetcherJob.perform_now(searchgov_url: searchgov_url)
It's a little more wordy than just SearchgovUrlFetcherJob.perform_now(searchgov_url)
, but it gives us more flexibility in case we need to add more args down the line. Sorry for making this specification late in the game; I was thinking I didn't mind the inconsistency with the other jobs, but I do think we should make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. So I now have it that perform
uses a keyword argument. Since the args
variable is still necessary, would it be cleaner to write SearchgovUrlFetcherJob.perform_now(args)
in the subject
declaration in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
@@ -1,7 +1,7 @@ | |||
require 'spec_helper' | |||
|
|||
describe SearchgovUrlFetcherJob do | |||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(args[:searchgov_url]) } | |||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(searchgov_url) } | |||
let!(:searchgov_domain) { SearchgovDomain.create(domain: 'agency.gov', status: '200') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this variable used anywhere...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this variable. It seemed to be necessary before when I was trying to test the fetching of a record, but now that I've changed that to what you recommended it isn't needed anymore.
@@ -60,7 +60,8 @@ | |||
'in the twitter govbox' => '#twitter_govbox', | |||
'in the site header' => '.l-site-header', | |||
'in the active site main navigation' => '.l-site-nav.main .active', | |||
'in the active site sub navigation' => '.l-site-nav.sub .active' | |||
'in the active site sub navigation' => '.l-site-nav.sub .active', | |||
'in the SearchgovDomain URLs table' => 'tbody.messages #as_admin__searchgov_urls-messages' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing , that selector appears to be for just the section that holds the flash messages for that table. As we may want to use the in the SearchgovDomain URLs table
selector more generically, I'd try the as_admin__searchgov_urls-active-scaffold
div.
to have_enqueued_job(SearchgovUrlFetcherJob).on_queue("searchgov") | ||
it 'enqueues a searchgov_url_fetcher_job to the searchgov queue' do | ||
expect{ post :fetch, params }. | ||
to have_enqueued_job(SearchgovUrlFetcherJob).on_queue("searchgov") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing , TDD is helpful for bug fixes as well. This test doesn't actually test this change, because it's not checking the job arguments: e7b3adb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that test to make sure it only takes named arguments go in searchgov_url_fetcher_job_spec.rb
since that test is specifically about the functionality of the perform
function within the model? I've already got a test in that spec that tests that the perform
is invalid if it isn't given an argument -- should I just add another that tests that it's invalid if it's not given a named argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is covering the bug you fixed with e7b3adb. It ensures we're passing the args correctly to the job. You can do that by specifying the args in your test:
expect{ post :fetch, params }.
to have_enqueued_job(SearchgovUrlFetcherJob).on_queue("searchgov").
with(searchgov_url: searchgov_url)
It couldn't hurt to also make your searchgov_url_fetcher_job_spec.rb
spec more specific, by testing that it requires asearchgov_url
keyword argument.
# post :fetch, params | ||
# p response.body | ||
# end | ||
context 'when logged in as an admin' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing , you should be able to use the shared 'super admin logged in'
context for this:
https://github.com/GSA/search-gov/blob/master/spec/support/shared_sites_controller_shared_behavior.rb#L46
https://relishapp.com/rspec/rspec-core/docs/example-groups/shared-context
fixtures :users | ||
|
||
let(:admin) { users(:affiliate_admin) } | ||
let(:searchgov_url) { SearchgovUrl.create(url: 'agency.gov/test') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to use bang methods in your tests, to ensure that your setup steps don't fail silently: SearchgovUrl.create!(url: 'agency.gov/test')
|
||
describe SearchgovUrlFetcherJob do | ||
subject(:perform) { SearchgovUrlFetcherJob.perform_now(args) } | ||
let!(:searchgov_url) { SearchgovUrl.create(url: 'https://agency.gov/') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment re. bang methods
@@ -15,6 +15,11 @@ | |||
to raise_error(ArgumentError) | |||
end | |||
|
|||
it 'must have a named argument' do | |||
expect { SearchgovUrlFetcherJob.perform_now searchgov_url }. | |||
to raise_error(ArgumentError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more specific you can be in your specs, the better. Here, you can specify the error message as well as the error class:
expect....to raise_error(ArgumentError, 'missing keyword: searchgov_url')
let(:params) do | ||
{ association: 'searchgov_urls', parent_scaffold: 'admin/searchgov_domains', | ||
searchgov_domain_id: searchgov_url.searchgov_domain.id, id: searchgov_url.id } | ||
end | ||
|
||
context 'when logged in as an admin' do | ||
include_context 'super admin logged in' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
it 'enqueues a searchgov_url_fetcher_job to the searchgov queue' do | ||
expect{ post :fetch, params }. | ||
to have_enqueued_job(SearchgovUrlFetcherJob).on_queue("searchgov") | ||
to have_enqueued_job(SearchgovUrlFetcherJob).on_queue("searchgov"). | ||
with(searchgov_url: searchgov_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
include_context 'super admin logged in' do | ||
describe '#fetch' do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing - I just noticed this. Please remove this extra line break
describe Admin::SearchgovUrlsController do | ||
fixtures :users | ||
|
||
let(:admin) { users(:affiliate_admin) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanewing , now that you're using the shared context, this line can be removed
@@ -3,7 +3,6 @@ | |||
describe Admin::SearchgovUrlsController do | |||
fixtures :users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops - I missed this too! You can remove this line and the line break following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My specs are failing when I comment that line out, unless I transfer that line (fixtures :users
) into the shared context itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the users
fixtures are used throughout the various shared contexts in spec/support/shared_sites_controller_shared_behavior.rb
, it makes sense to add the fixtures :users
line to the top of that file.
@ethanewing , please move |
Not finished: searchgov_url_fetcher_job still WIP