Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

Status: Active » Needs review
FileSize
1.6 KB

This doesn't get them all passing, but allows them all to run.

Berdir’s picture

Status: Needs review » Needs work

Can we install the config with a manual call from setUp(), that's a bit too much magic for me.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.29 KB

Re-rolled, I didn't need the installConfig() override method.

This doesn't pass yet but currently you get a fatal error even when just trying to discover tests, due to the moved parent class. Committing this is as stop-gap fix for that. I get at least somewhere into actually running the test now with this.

  • Berdir committed 74df2e1 on 8.x-1.x
    Issue #2595077 by jhedstrom, Berdir: Get tests running again
    
Berdir’s picture

Status: Needs review » Active

Committed this, back to active.

  • Berdir committed 32a7b3c on 8.x-1.x
    Issue #2595077 by jhedstrom, Berdir: Fixed test namespace
    
Berdir’s picture

Tests look a lot better now with #2636270: Head broken committed too. A bunch of facet tests fail, some regression tests fail, e.g. related to case sensitivity but looks fine otherwise.

Don't know enough about those topics to fix these tests myself.

borisson_’s picture

Status: Active » Needs review

I'll get a solr instance working again locally so I can get all of them to pass, at least this patch will be useful because of the changes in the config schema.

borisson_’s picture

FileSize
2.33 KB
borisson_’s picture

FileSize
609 bytes
2.28 KB

I was too eager in my c/p of the db index schema. This should be better.

borisson_’s picture

I can't figure out why the testcode doesn't return expected facets, while the code does work in my local test-setup.

I did add some extra assertions in search api to help me figure out why this was failing, those are green though. So at least that's working as expected. #2658120: Expand Backend test.

When running tests, the $resultset->getFacetSet() basically returns an empty string with a count of 2, while on my actual test-site the correct facets are being returned.

object(Solarium\QueryType\Select\Result\Facet\Field)#1952 (1) {
  ["values":protected]=>
  array(1) {
    [""]=>
    int(2)
  }
}
borisson_’s picture

FileSize
5.23 KB
7.24 KB

I still can't figure it out, but at least here's a patch with an extra test that only tests the facets and more comments in the code of what's happening and supposed to happen.

I will come back to this; but probably not today/tomorrow.

borisson_’s picture

FileSize
15.99 KB
23.23 KB

This is driving me insane.

I added an integration test to make sure the solr integration in the backend works. This is a copy of the integration test in search api; with a whole bunch of tests removed. I don't think it's needed to test all edge cases of the backend. The strict config checking there made me find a small issue with the config schema. Fix attached.

borisson_’s picture

FileSize
2.2 KB
24.31 KB

More code in the tests. Still not green. I think the problem is in SearchApiSolrBackend::setFacets.

Berdir’s picture

+++ b/config/schema/search_api_solr.backend.schema.yml
@@ -67,6 +67,9 @@ search_api.backend.plugin.search_api_solr:
           label: 'Use Get or Post for the Solr Queries'
+        site_hash:
+          type: string
+          label: 'Site hash'

This is a checkbox/boolean I think, not a string.

borisson_’s picture

FileSize
625 bytes
24.33 KB

Fixed #16.

Berdir’s picture

Haven't fully reviewed everything yet. Do you think we should commit this and then continue with fixing the remaining test fails?

borisson_’s picture

@Berdir, the latest patch doesn't break as hard as without the patch, but tests are still red. So I don't think it really matters a lot.

drunken monkey’s picture

I don't think it'll happen in the next week, but if I get the time to take a look at this – how do I actually run the tests? Does it work just by running the normal test suite, or are there special steps to do?

borisson_’s picture

You need to have a solr instance at: http://localhost:8983/solr/d8/ but that's all you need to run the tests.

drunken monkey’s picture

OK, thanks!
Should we maybe add that to the test class documentation?

borisson_’s picture

I think it should be added in the readme.txt instead of the testclass, so we won't have to copy that text over to other test classes.

drunken monkey’s picture

FileSize
26.05 KB
43.65 KB

The attached works for me. Please test/review!

  • Berdir committed 74df2e1 on 2595077--fix_tests
    Issue #2595077 by jhedstrom, Berdir: Get tests running again
    
  • Berdir committed 32a7b3c on 2595077--fix_tests
    Issue #2595077 by jhedstrom, Berdir: Fixed test namespace
    
drunken monkey’s picture

Created a Github PR, too, so we can see whether the tests really pass for all environments. (Though, since they currently aren't running at all, I guess we might as well just have committed it and seen afterwards.)

drunken monkey’s picture

Status: Needs review » Needs work

"Drupal\Tests\search_api_solr\Kernel\SearchApiSolrTest 1 passes"

That seems like it failed to find the Solr instance, or Travis failed to install it. That's something I know nothing about, so would be great if someone else could take a look.

drunken monkey’s picture

Status: Needs work » Active

Committed.
Setting back to "Active" for fixing Travis to correctly detect passing/failing build.

Berdir’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
8.17 KB

Ok, that now seems to be working as expected.

I thought that the manual parsing is no longer required and run-tests.sh should return 1/0 correct, but it somehow doesn't yet.

Note that we currently have an error reported at the end of the test that is not properly checked. This is actually great because it means that we basically have test coverage for #2685035: Avoid redundant data in configuration already. So once this is in, we just have to re-check the patch there (with a PR) and possibly add an explicit assertion somewhere, it just fails through the error log parsing.

I also had to change the regex that's looking for errors so it matches those.

Berdir’s picture

Hm, that patch isn't what I expected.

Berdir’s picture

Status: Needs review » Fixed

Ok, committed. I think we can close this now, despite still having some test fails, we'll take care of that in the referenced issue.

  • Berdir committed b2d0201 on 8.x-1.x
    Issue #2595077 by Berdir: Improve and fix travis.yml
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.