-
Notifications
You must be signed in to change notification settings - Fork 45
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
[MAINTENANCE] Add tests for Common package #953
Conversation
5ca0c7e
to
5e2c8de
Compare
f1636f0
to
83bb871
Compare
*/ | ||
public function canPrepareAndSubmit() | ||
{ | ||
$this->markTestSkipped('Does not work in combination with other tests.'); |
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.
Hm, a skipped test is quite useless... There should be a way to make this work even in combination with the other tests. Where exactly does the test fail?
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 reenabled the test. It fails on line 55 at the first assert
using the SolrSearch
object from line 53. I could not find any difference in any variables accesses in the tested SolrSearch::prepare
method.
I also switched to a different Solr core, but that didn't change the result.
However I'm not sure about the DocumentRepository
passed to the constructor of the SolrSearch
– I'm don't fully understand how the DocumentRepository
object is created (or maybe reused?).
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 am sorry, but I can't help you with that. Maybe @beatrycze-volk can give more insight into using the DocumentRepository
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.
I am sorry, but I can't help you with that. Maybe @beatrycze-volk can give more insight into using the
DocumentRepository
correctly?
Actually more suitable person to answer this question would be @chrizzor as he has implemented this class.
I fixed the PHPStan issue in |
I fixed the test. It looks like some database query were cached, that were necessary to determine the correct solr core. The solr cores are now determined more directly. |
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.
Great job! Thank you very much!
Now you only need to update your branch to master and then I can merge it! |
9af19ad
to
3eb9d12
Compare
No description provided.