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

Added feature to use custom methods for filtering #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xenco
Copy link

@xenco xenco commented Dec 17, 2017

See the added part in README for further explanation.

@timgws
Copy link
Owner

timgws commented Dec 18, 2017

Hi xenco!

Thanks for your first commit to the project!

To help ensure that there is high quality code inside this project, you will note that we have automated testing. This is provided by a service called Travis.

When a new pull request is made (or changes are made to a branch with a pull request), Travis will automatically try running the code base in a few different configurations.

This automatic test has failed.

2) timgws\test\JoinSupportingQueryBuilderParserTest::testDateBetween
timgws\QBParseException: Field (dollar_amount) is in no list

/home/travis/build/timgws/QueryBuilderParser/src/QueryBuilderParser/QBPFunctions.php:276
/home/travis/build/timgws/QueryBuilderParser/src/QueryBuilderParser/QueryBuilderParser.php:251
/home/travis/build/timgws/QueryBuilderParser/src/QueryBuilderParser/JoinSupportingQueryBuilderParser.php:70
/home/travis/build/timgws/QueryBuilderParser/src/QueryBuilderParser/QueryBuilderParser.php:80
/home/travis/build/timgws/QueryBuilderParser/src/QueryBuilderParser/QueryBuilderParser.php:60
/home/travis/build/timgws/QueryBuilderParser/tests/JoinSupportingQueryBuilderParserTest.php:299

It appears that some of the logic for comparing fields in the database may be broken, as most of the tests have failed.

To run these tests manually on your computer when developing, run these commands inside the project folder:

composer install --prefer-source --no-interaction --dev
vendor/bin/phpunit --testsuite MySQL --coverage-clover ./tests/logs/clover.xml

Also, feel free to ask for any help if you need it!

@xenco
Copy link
Author

xenco commented Dec 18, 2017

It doesn't break the default functionality anymore.
But I'm not sure how to test the added features. I can't just check the produced (prepared) queries, because they need the bindings, which themselves need some test data sets.
How would I create a test database with migration and seeding for sqlite:memory in isolation? Any help is appreciated.

@timgws
Copy link
Owner

timgws commented Jan 15, 2018

I can't just check the produced (prepared) queries, because they need the bindings

You actually should be able to do that. I don't think that doing a full test is really all that necessary.

We only need to make sure that we correctly generate queries that we expect. We don't need to do an end to end test, as that would be the responsibility of the project that is adding this library as a dependency.

Here is an example test that I have written: https://github.com/timgws/QueryBuilderParser/blob/master/tests/QueryBuilderParserTest.php#L272-L286

@timgws
Copy link
Owner

timgws commented Jan 15, 2018

That being said, here is an instance where the project does test bindings:

/**
* Test for #21 (Cast datetimes and add 'not between' operator)
*/
public function testDateNotBetween()
{
$incoming = '{ "condition": "AND", "rules": [ { "id": "needed_by_date", "field": "needed_by_date", "type": "date", "input": "text", "operator": "not_between", "value": [ "10/22/2017", "10/28/2017" ] } ], "not": false, "valid": true }';
$builder = $this->createQueryBuilder();
$qb = $this->getParserUnderTest();
$qb->parse($incoming, $builder);
$this->assertEquals('select * where `needed_by_date` not between ? and ?', $builder->toSql());
$bindings = $builder->getBindings();
$this->assertCount(2, $bindings);
$this->assertInstanceOf("Carbon\\Carbon", $bindings[0]);
$this->assertInstanceOf("Carbon\\Carbon", $bindings[1]);
$this->assertEquals(2017, $bindings[0]->year);
$this->assertEquals(22, $bindings[0]->day);
$this->assertEquals(28, $bindings[1]->day);
}

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

2 participants