Fix _almost_ all qiita db tests #1099
Conversation
|
I thought we agreed that you were going to fix each test file in separate pull requests. This is way too large to review. |
|
This is including all the changes in the previous PR. The actual changes here are minimal... |
|
That was not made clear anywhere in the pull request. Please remember to add that information for future pull requests. |
|
Will do, sorry about that. I'll update the description to reflect that. |
…fix-qiita-db-tests
|
Just pulled from upstream! |
| # column names from required_sample_info table | ||
| required_cols = set(get_table_cols("required_sample_info")) | ||
| # column names from study_sample table | ||
| required_cols = set(get_table_cols("study_sample")) |
There was a problem hiding this comment.
This check isn't required any more, since all we have in the table now is study id and sample id, correct?
There was a problem hiding this comment.
You're correct. Removing! Your feedback on this code is really useful, as I had some problems understanding it...
|
Couple of comments. |
| exp_samp_sql = ("SELECT r.sample_id,r.host_subject_id FROM " | ||
| "qiita.required_sample_info r JOIN qiita.sample_{0} sa" | ||
| exp_st_sql = ("SELECT study_id FROM qiita.study_sample_columns " | ||
| "WHERE lower(column_name) = lower('host_subject_id') " |
There was a problem hiding this comment.
There's no need to have lower('host_subject_id') is there?
There was a problem hiding this comment.
i.e. there's no need to call lower on that string.
There was a problem hiding this comment.
That is actually built programmatically, and lowers everything it gets so that search is case-insensitive.
|
@josenavas, looks in really good shape, I only had two rather minor comments 👍 on my end granted that @squirrelo's comments are addressed. |
|
Thanks guys, ready for another round! |
|
👍 |
|
@squirrelo, would you mind merging if all looks good to you? |
|
I'm getting "Internal Server Error, please try again later" When using search now. Can you look into this and fix it? |
|
Nevermind, local system issue. Merging. |
Fix _almost_ all qiita db tests
Built on top of #1075
Fixes most of the qiita_db tests. The only tests that are not fixed are the analysis.py tests. The reason is that fixing those tests requires a bit more work and I do not want to blow-up the size of this PR.