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

Feature index data #15

Merged

Conversation

anusharanganathan
Copy link
Collaborator

  1. Merge conflicts should disappear after solr_config PR is accepted (hoping)
  2. The rake task to index fixtures takes about 5 minutes (run along with the install rake task) - may be I need to prune the number of fixture objects to just 1 or 2 (rather than 4)
  3. I have pending tests - not sure how to test against SOLR (it's all mostly written)

@anusharanganathan
Copy link
Collaborator Author

Okay I need help on how to fix this. I included the webmock gem to stub http requests in the rspec tests. That is also blocking calls to github during bundle exec rake. I thought webmock was scoped to work only for rspec tests. I tried allowing access to github (anusharanganathan@2fa90e2) and it still failed.

@jkeck jkeck self-assigned this Jan 4, 2016
@jkeck
Copy link
Contributor

jkeck commented Jan 4, 2016

Hmm, I haven't used WebMock myself personally so can't offer any tips from experience off the top of my head.

Looking at the error and poking around the docs I can think of two solutions to at least evaluate.

  1. Only stub the necessary requests in tests and don't stub all HTTP requests (not sure if WebMock will allow this easily or not).
  2. Use WebMock.allow_net_connect! in the context of the rake task that fetches the jetty zip. (Rake::Task['jetty:download'].invoke)

If none of that helps I can certainly pull it down and try some things out, but not sure how helpful I will be w/o being very facile w/ WebMock myself.

@anusharanganathan
Copy link
Collaborator Author

Oh one other thing, I have a few pending tests in data_indexer_spec.rb because I can't get the solr stub to work and so the tests are failing.

# it 'should have received adds' do
# expect(stub_solr).to receive(:add).at_least(50).times.and_return(true)
# expect(stub_solr).to receive(:commit).once_and_return(true)
# end
Copy link
Contributor

Choose a reason for hiding this comment

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

@anusharanganathan, I'm not sure this will work w/ all of your tests here, but a quick experiment pulling this down locally I was able to get this test to pass by making a few minor changes.

  1. @di.run out of the before block and at the end of the it block.
  2. Changed stub_solr to @stub_solr
  3. Change .once_and_return to .once.and_return
describe '#run index_csv' do
  before do
    @di = DataIndexer.new('test collection', manifest_csv_file, nil)
  end
  it 'should have received adds' do
    expect(@stub_solr).to receive(:add).at_least(50).times.and_return(true)
    expect(@stub_solr).to receive(:commit).once.and_return(true)
    @di.run
  end
end

Some of the other tests may need other changes, but hopefully that gets you moving forward.

@anusharanganathan
Copy link
Collaborator Author

@jkeck Thanks a lot. I implemented the changes you suggested and fixed all pending tests in data indexer.

@@ -15,7 +15,7 @@ development:
url: <%= ENV['SOLR_URL'] || "http://127.0.0.1:8984/solr/blacklight-core" %>
test: &test
adapter: solr
url: <%= "http://127.0.0.1:#{ENV['TEST_JETTY_PORT'] || 8888}/solr/blacklight-core" %>
url: <%= "http://127.0.0.1:#{ENV['TEST_JETTY_PORT'] || 8984}/solr/blacklight-core" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason we're changing to port 8984 for the test env here?

@jkeck
Copy link
Contributor

jkeck commented Jan 6, 2016

👍 looks good to me. I have 2 minor comments, neither of which is particularly pressing. More than happy to ship it when you're ready.

@anusharanganathan
Copy link
Collaborator Author

Ah thanks for noticing the test port. That was me just experimenting. I've changed it back to the default.

Also your comment on the if statement made me look at my code and tests again and I realized I did not test the methods for no parameters passed. I changed the if statement and added a few more tests.

jkeck pushed a commit that referenced this pull request Jan 7, 2016
@jkeck jkeck merged commit 3d901db into sul-dlss-deprecated:master Jan 7, 2016
@anusharanganathan anusharanganathan deleted the feature_index_data branch January 7, 2016 13:49
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

3 participants