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

Crate filter tests #1277

Merged
merged 4 commits into from
Jun 5, 2017
Merged

Crate filter tests #1277

merged 4 commits into from
Jun 5, 2017

Conversation

gramanas
Copy link
Contributor

@gramanas gramanas commented Jun 4, 2017

Following PR #1263 here are four five tests for the crate filters.
The code is commented and what is happening should be pretty self explanatory.

Four new tests for the crate filter in the
searchqueryparsertest.cpp
@Be-ing
Copy link
Contributor

Be-ing commented Jun 4, 2017

Looks good so far. I thought the tests would have to clean up the database at the end of each test adding to the DB, but ~MixxxTest deletes all the temporary test files between each test run, so that isn't necessary.

It would be good to add a test combining crate filters like "crate:testCrate1 crate:testCrate2" and "crate:testCrate1 -crate:testCrate2". You can put these in the same test so you don't have to duplicate setting up the test crates and adding the test tracks. Add two tracks to each crate, with one track being in common in both crates.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 5, 2017

LGTM. @uklotzde, any comments?

@uklotzde
Copy link
Contributor

uklotzde commented Jun 5, 2017

LGTM. It would be possible to reduce the amount of redundant code among the very similar setup procedures for different tests. Just a minor remark, nothing that prevents merging this PR.

Thanks for completing the new feature with appropriate tests, Anast!

@daschuer
Copy link
Member

daschuer commented Jun 5, 2017

Thank you all working on this!

@daschuer daschuer merged commit 13720e3 into mixxxdj:master Jun 5, 2017
@gramanas gramanas deleted the CrateFilterTests branch June 5, 2017 12:39
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

4 participants