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

Updated tests for esquery.py #64

Merged
merged 1 commit into from Jun 13, 2018
Merged

Conversation

aswanipranjal
Copy link
Contributor

Divided test_get_query_agg_ts and test_get_query_basic into 2 parts in test_esquery.py.
The tests might fail because they were changed in #63.

Copy link
Contributor

@jgbarah jgbarah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aswanipranjal did you notice that tests are failing? Could you please have a look at the Travis output, and try to fix them?

@aswanipranjal aswanipranjal mentioned this pull request Jun 4, 2018
@aswanipranjal aswanipranjal force-pushed the split-test branch 2 times, most recently from 3cb2503 to f376a3f Compare June 7, 2018 05:22
@jgbarah
Copy link
Contributor

jgbarah commented Jun 7, 2018

The problem with the test is that json.loads can extract the list in the dictionary in any order, which makes the test fail. I suggest that you write a simple auxiliary function to order the list (say, alphabetic order), and just apply it to the lists after json.loads... (of course, test_agg_dict1 should match this order).

@aswanipranjal aswanipranjal force-pushed the split-test branch 2 times, most recently from ed5109a to 59988d4 Compare June 9, 2018 10:50
@aswanipranjal
Copy link
Contributor Author

@jgbarah please review.

@@ -64,7 +69,7 @@ def __get_query_filters(cls, filters={}, inverse=False):
# trying to use es_dsl only and not creating hard coded queries
query_filters.append(Q('match_phrase', **params))

return query_filters
return sorted(query_filters, key=lambda x: sort_order(x))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to do this in tests, instead of here. The main reason is performance (this is not really needed when working as usual, but the performance penalty of ordering will be paid every time you query), but also that the code does not ready this added complexity: tests do need, but not this code....

@aswanipranjal
Copy link
Contributor Author

I changed the tests to sort the list only during the tests and not other wise in esquery.py

The reason the tests are failing is this:
The function

__get_query_filters

takes in a dictionary containing the filters to be applied to the Search object and returns a list of elasticsearch_dsl Query object (the filters were converted to these Query objects).
When we are testing this function, we can sort the output list containing these Query objects.

But when this function is called internally (in esquery.py) in __get_query_basic, then there is no way that we can sort this list externally, and for testing the test_get_query_basic we will be unable to sort the list to get the queries in ordered form and thus the test will fail.

We will have to apply the sorting function in __get_query_filters only so that when it is called inside __get_query_basic, it sorts the list and adds the filters in ordered manner so that the test_get_query_basic has the filters in ordered manner and hence it passes the test.

The same thing happens in get_aggs and test_get_aggs.

Divided test_get_query_agg_ts and test_get_query_basic into 2 parts
Sorting the list generated by get_query_filters to pass the tests
@aswanipranjal
Copy link
Contributor Author

aswanipranjal commented Jun 13, 2018

@jgbarah I was using self.filters as an OrderedDict previously, but I suppose the dictionary of filters that I was passing into the ordered dict had the key-value pairs in different order. It was a silly mistake on my part. I suppose this is it?

Copy link
Contributor

@jgbarah jgbarah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

@jgbarah jgbarah merged commit d859cad into chaoss:master Jun 13, 2018
@aswanipranjal aswanipranjal deleted the split-test branch July 8, 2018 07:28
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