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

[WIP] re-enable solr-specific tests #1409

Merged
merged 32 commits into from
Jun 15, 2017
Merged

[WIP] re-enable solr-specific tests #1409

merged 32 commits into from
Jun 15, 2017

Conversation

icarito
Copy link
Member

@icarito icarito commented May 10, 2017

This is to replace #1386 with a branch in plots2 repository in order to enable coveralls.

  • all tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request is descriptively named with #number reference back to original issue

@icarito
Copy link
Member Author

icarito commented May 10, 2017

  • figure out why Coveralls isn't running on here, and get it running so we can see if coverage drops - maybe by reopening this from publiclab/plots2?
  • document why 23-hour reindex is needed, or fix the toggle to make this unnecessary
  • fix tests on test/solr/search_record_test.rb:50 to ensure unique results are actually returned
    • was this bc it was not actually indexing? So, maybe if we get Solr indexing properly, this will pass.
  • re-enable test.rake "solr-specific" tests -- this may be due to those extra tests running in a subshell...
  • confirm that Coveralls now sees solr-specific tests, and maintains 86% coverage
    • is Coveralls rate-limited, and we ran out?
  • resolve test failure at test/solr/searches_controller_test.rb:23 (see log below)
  • ensure Node.search returns things, and unique things, at end of test/solr/search_record_test.rb
  • add # this is required to get results to return: and docs on needed adjust_solr_params do |params| // params[:qf] = nil // end for every Node.search
  • Once it does, we want to try turning off Solr and ensuring the site doesn't break (as the new toggle code should allow for).
    • Toggle code is not working -- why?
  • re-enable solr on production

Then, the rest of this:

@PublicLabBot
Copy link

PublicLabBot commented May 10, 2017

3 Messages
📖 @icarito Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution!
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.
📖 Pull Request is marked as Work in Progress

Generated by 🚫 Danger

@icarito
Copy link
Member Author

icarito commented May 10, 2017

Great so we can check the coveralls issue now. I'll be closing #1386 and follow this PR instead, @jywarren

@icarito icarito force-pushed the reenable-solr-tests branch 2 times, most recently from fa3e4b8 to e5baaf8 Compare May 10, 2017 15:18
@icarito
Copy link
Member Author

icarito commented May 10, 2017

Yay, figured out to fix the DISABLE_SOLR_CHECK (renamed) env var toggle to properly reindex in 47 minutes.

@jywarren
Copy link
Member

jywarren commented May 10, 2017 via email

@icarito
Copy link
Member Author

icarito commented May 10, 2017

So it works:
http://branch1.laboratoriopublico.org/searches/55
Searches feel slow to me, possibly from our pre-search solr check.

@icarito icarito changed the title re-enable solr-specific tests [WIP] re-enable solr-specific tests May 10, 2017
@icarito
Copy link
Member Author

icarito commented May 10, 2017

This is the result of running solr tests in travis:

Command failed with status (1): [ruby -I"lib:test" -I"/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib" "/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb" "test/solr/search_record_test.rb" "test/solr/searches_controller_test.rb" ]
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/testtask.rb:109:in `block (3 levels) in define'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils.rb:57:in `call'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils.rb:57:in `sh'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils_ext.rb:37:in `sh'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils.rb:96:in `ruby'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils_ext.rb:37:in `ruby'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/testtask.rb:105:in `block (2 levels) in define'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/file_utils_ext.rb:58:in `verbose'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/testtask.rb:101:in `block in define'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:240:in `call'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:240:in `block in execute'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:235:in `each'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:235:in `execute'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:179:in `block in invoke_with_call_chain'
/usr/local/lib/ruby/2.1.0/monitor.rb:211:in `mon_synchronize'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:172:in `invoke_with_call_chain'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:165:in `invoke'
/usr/local/lib/ruby/gems/2.1.0/gems/railties-3.2.22.4/lib/rails/test_unit/testing.rake:50:in `block in <top (required)>'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:240:in `call'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:240:in `block in execute'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:235:in `each'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:235:in `execute'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:179:in `block in invoke_with_call_chain'
/usr/local/lib/ruby/2.1.0/monitor.rb:211:in `mon_synchronize'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:172:in `invoke_with_call_chain'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/task.rb:165:in `invoke'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:150:in `invoke_task'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `block (2 levels) in top_level'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `each'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `block in top_level'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:115:in `run_with_threads'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:100:in `top_level'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:78:in `block in run'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:176:in `standard_exception_handling'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/application.rb:75:in `run'
/usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/bin/rake:33:in `<top (required)>'
/usr/local/bin/rake:23:in `load'
/usr/local/bin/rake:23:in `<main>'
Tasks: TOP => test:single
[Coveralls] Using SimpleCov's default settings.
Loaded suite /usr/local/lib/ruby/gems/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader
Started
....E
===============================================================================
Error: test_should_get_search_test_action(SearchesControllerTest): JSON::ParserError: 757: unexpected token at 'null'
test/solr/searches_controller_test.rb:23:in `block in <class:SearchesControllerTest>'
===============================================================================
....
Finished in 2.07917866 seconds.
------
9 tests, 17 assertions, 0 failures, 1 errors, 0 pendings, 0 omissions, 0 notifications
0% passed
------

@icarito icarito force-pushed the reenable-solr-tests branch from af44c53 to 5994757 Compare May 10, 2017 18:55
@jywarren
Copy link
Member

Huh, cool -- great work! But looks like the error at test/solr/searches_controller_test.rb:23 is distinct, maybe new? Shall we add that to the checklist too?

@jywarren
Copy link
Member

But i got a bunch of email notifications about not working when Solr is off -- but the comments are gone now, was that resolved? Does it now work whether Solr is on or off?

@icarito
Copy link
Member Author

icarito commented May 10, 2017

Yes sorry - i deleted those messages - initially I accidentally double posted. Then I removed one copy. Then I looked at a different tab not knowing it and saw the duplicate message, so I deleted it, but turns out I deleted both messages. Sigh.

Summarizing, yes, stopping Solr causes an error. :-/

@icarito
Copy link
Member Author

icarito commented May 12, 2017

Here's the error we get when Solr is off:

Started GET "/searches/56" for 181.66.127.24 at 2017-05-12 15:17:55 +0000
Processing by SearchesController#show as HTML
  Parameters: {"id"=>"56"}
Completed 500 Internal Server Error in 67.8ms

RSolr::Error::ConnectionRefused (Connection refused - {:data=>"fq=type%3ANode&fq=updated_at_s%3A%7B*+TO+%222017%5C-05%5C-12%5C+15%5C%3A17%5C%3A55%5C+UTC%22%7D&q=balloon&fl=*+score&defType=edismax&start=0&rows=30&facet=true&f.updated_month_s.facet.mincount=1&facet.field=updated_month_s", :method=>:post, :params=>{:wt=>:ruby}, :query=>"wt=ruby", :headers=>{"Content-Type"=>"application/x-www-form-urlencoded; charset=UTF-8"}, :path=>"select", :uri=>#<URI::HTTP:0x007f6404245148 URL:http://pad.publiclab.org:8983/solr/default/select?wt=ruby>, :open_timeout=>nil, :read_timeout=>120, :retry_503=>nil, :retry_after_limit=>nil}):     app/models/search_record.rb:34:in `notes'                                                                                                                    app/controllers/searches_controller.rb:57:in `show'
                                                                                                                                                              

@icarito
Copy link
Member Author

icarito commented May 12, 2017

Here's another coverage tool that perhaps even is used underneath coveralls?
https://github.com/colszowka/simplecov
It has pretty tables! ;-)

@jywarren jywarren force-pushed the reenable-solr-tests branch from adbd92f to 9053929 Compare May 12, 2017 16:47
@jywarren
Copy link
Member

  • Node.search is now being tested, could fail as of last couple commits.
  • we are trying to reindex embedded Solr in Travis rake test:solr, but that might not work?
  • it could need some sleep
  • maybe ditch Coveralls for simplecov

@jywarren
Copy link
Member

Hmm, some green and red there, nice!

BTW does Coveralls just use SimpleCov? https://travis-ci.org/publiclab/plots2/builds/231645129#L2782 says:

[Coveralls] Using SimpleCov's default settings.

@jywarren
Copy link
Member

missed an end -- fixed it and added one more commit!

@jywarren
Copy link
Member

OK, reindex failed but because: sh: 1: docker-compose: not found on https://travis-ci.org/publiclab/plots2/builds/231657953#L2805

I think this is because we actually want to send it to embedded Solr, so we could just run rake SOLR_DISABLE_CHECK=1 sunspot:reindex without the docker compose stuff. Trying that.

@jywarren
Copy link
Member

Nice! Solr indexed: [##################################] [72/72] [100.00%] [00:01] [00:00] [57.72/s]

Trying to resolve another test failure now...

@jywarren
Copy link
Member

OK, in both cases it's not returning any results:


===============================================================================
Failure:
  <[]> expected to be != to
  <[]>.
test_should_get_search_test_action(SearchesControllerTest)
test/solr/searches_controller_test.rb:25:in `block in <class:SearchesControllerTest>'
===============================================================================
.F
===============================================================================
Failure: <false> is not true.
test_Node.search_for_two_different_key_words_returns_different_results(SearchRecordTest)
test/solr/search_record_test.rb:54:in `block in <class:SearchRecordTest>'
===============================================================================

However, Node.search does return results, so maybe we should just echo those out to be ...

OH! this is where we need htat extra parameter!

@icarito
Copy link
Member Author

icarito commented May 12, 2017 via email

@@ -59,6 +59,8 @@ class SearchRecordTest < ActiveSupport::TestCase
#with(:updated_month, month) if month.present?
#paginate :page => 1, :per_page => 10
end
puts solr_search_1.results
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, i have to wrap up for the day, but my attempt to output the results here didn't turn into anything. We could try puts solr_search_1.results.inspect instead...?

@jywarren
Copy link
Member

Some hints on coveralls vs. simplecov here: http://technology.indiegogo.com/tag/simplecov/ -- though we should maybe just break out a separate issue.

@jywarren
Copy link
Member

jywarren commented Jun 1, 2017

OK, after today's work, a checklist is:

  • we're outputting all the 72 indexed nodes from the Solr auto-testing, to see why "Chicago" and "pectro" are not returning any records in search_record_test.rb
  • @icarito is trying to get a rails console running with the test data so we can manually query it and see why it's not working in the tests.

We did confirm that in the staging server, these exact queries do return results, so the Solr test failures are confusing. All are related to Node.search do ... Solr queries.

@jywarren jywarren force-pushed the reenable-solr-tests branch from 602b473 to fe76c6f Compare June 14, 2017 20:14
@jywarren
Copy link
Member

Rebased and attempted a fix

@jywarren
Copy link
Member

aha -- hmm, this time, we dropped coverage again! how/why!?

@jywarren
Copy link
Member

Awesome, this worked perfectly!

Solr search service offline

We can write a wrapper for Node.search which checks this first, such as Node.search_if_available!

@jywarren
Copy link
Member

@icarito, if I add this, would you be ready to reenable Solr on production?

@icarito
Copy link
Member Author

icarito commented Jun 14, 2017

GREAT! pad.publiclab.org is ready with a Solr container.
I believe pushing to staging should work cleanly :-)
🏆 🌴 🎆 🥇 😀

@icarito
Copy link
Member Author

icarito commented Jun 14, 2017

Looking forward to put this in production. :-)

@jywarren
Copy link
Member

Fingers X, i've implemented on-read toggles for all instances of Node.search.

@jywarren
Copy link
Member

We could test this out on Sunday if you like -- assuming nothing else comes up. I want to be very thorough about testing with Solr on and off, in staging.

@jywarren
Copy link
Member

And in production actually!

@jywarren
Copy link
Member

Wheeee!

@jywarren
Copy link
Member

Nice, and both http://branch1.laboratoriopublico.org/ and http://branch1.laboratoriopublico.org/searches/test/test?q=About load normally -- with Solr off. I think this is ready for a merge!

- docker-entrypoint.sh
- solr-precreate
- default
# solr:
Copy link
Member

Choose a reason for hiding this comment

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

Also, just checking -- do we need to re-enable this?

Copy link
Member

Choose a reason for hiding this comment

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

also any relation to #1428 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in production or staging as we're planning to deploy the solr container in pad.publiclab.org.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'll add a separate docker-compose-production.yml for this, so we might as well reenable this here.

@jywarren
Copy link
Member

jywarren commented Jun 15, 2017 via email

@icarito
Copy link
Member Author

icarito commented Jun 15, 2017

It should work as-is, if I understood correctly. I'm going to be afk for the next few hours.
Regards,
Sebastian

@jywarren
Copy link
Member

Ah, so i can merge this?

@icarito
Copy link
Member Author

icarito commented Jun 15, 2017

Yes, we can merge the commented version, I believe. Sunspot.yml doesn't point to the solr container but to localhost, and it's off by default as Solr is only optional. I'm adding an explanatory comment, though.

Just also add a note for the sake of thoroughness, the Solr schema creation process (which we assumed occurred during rake db:setup) showed differences when done with the embedded engine vs the container (to the point where queries required different parameters). It would be good to take these into consideration when putting in production, and even better, to understand why.

@jywarren jywarren merged commit 6de3480 into master Jun 15, 2017
@jywarren
Copy link
Member

Ok!!!

@icarito
Copy link
Member Author

icarito commented Jun 15, 2017

Oh wow that was fast I was committing the additional comment on docker-compose.yml -

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.

3 participants