feat(other): integrate advanced search into saved searches module set #1738
Conversation
|
@IrAlfred thanks, it looks good but can you please add some unit/integration tests to cover the new functionality? |
IrAlfred
force-pushed
the
integrate-advanced-search-into-saved-searches
branch
2 times, most recently
from
77eebbe to
abce26c
Compare
IrAlfred
force-pushed
the
integrate-advanced-search-into-saved-searches
branch
from
abce26c to
afa4d24
Compare
| public function setUp(): void { | ||
| require_once __DIR__.'/../../bootstrap.php'; | ||
| require_once APP_PATH.'modules/core/modules.php'; | ||
| require_once APP_PATH.'modules/saved_searches/modules.php'; |
There was a problem hiding this comment.
These three lines are inappropriately placed here. They should be part of the test suite bootstrap process.
There was a problem hiding this comment.
Currently, bootstrap.php is included directly in the test classes. For now, I've kept it because otherwise we'd have to modify all these classes and each time we'd either run the tests with the --bootstrap=bootstrap.php flag or include it in the phpunit.xml configurations.
What do you suggest for this ?
There was a problem hiding this comment.
Hmm, I see. Well, that's a bit weird to include bootstrap over and over again. Bootstrapping processes is meant to be run only once when the test suite starts, it sets up everything and then the tests are executed. setUp class methods are meant to set up the environment for the tests defined in that file and shouldn't do global bootstrapping tasks. I suggest you make sure bootstrap.php is called ones, put it in phpunit.xml, remove run-time require statements and make sure the bootstrap process includes all modules and files, so we don't have to do random require statements here and there.
There was a problem hiding this comment.
Can we do that in a separate PR as it's some kind of refactoring ?
There was a problem hiding this comment.
We won't merge this before the 2.5 release, so you have time to do it here. If you prefer, refactor in another branch and PR but then it will have conflicts as you will have to start from current master and you will end up having to resolve the conflicts.
IrAlfred
force-pushed
the
integrate-advanced-search-into-saved-searches
branch
from
afa4d24 to
c893f4b
Compare
IrAlfred
force-pushed
the
integrate-advanced-search-into-saved-searches
branch
from
c893f4b to
9d1500b
Compare
IrAlfred
force-pushed
the
integrate-advanced-search-into-saved-searches
branch
from
9d1500b to
169e1b4
Compare
2 participants
Related issue #306