Fix attempt to download non-existing tables #812
Conversation
Due to the mutability of lists, this code was modifying all the elements of the dictionary. This resulted in an attempt to download tables that don't exist in the database.
There was a problem hiding this comment.
I believe that this does what it says on the tin. How difficult would it be to add a test?
If tables were a tuple, instead of a list, the original version would have worked as intended: lhs += rhs mutates lhs for mutable types (e.g. list) but rebinds it to a new object (lhs + rhs) for immutable types (e.g. tuple).
There was a problem hiding this comment.
Good catch, this PR does what it claims. To me is ok if you add a test, but I think changing the lists to tuples would be good enough.
Also, even though the test in download_test.py is skipped due to random misconnections, the dbname and table are fixed inside the function, which is parametrized. Could you fix this either removing the parametrization or the harcoded dbname? We may decide to activate the test in the future.
I'm thinking of a test that checks for the existence of the corresponding tables for each database. It's not perfect, as @gondiaz mentioned we have issues with the connection to the server. I can think of another test, but it's a bit trivial, I will push it and you can comment on that.
I will take a look into that, maybe it's the same as mentioned above. |
|
No, I had just switching to that (not commited yet). |
|
|
||
|
|
||
| @mark.parametrize('dbname', db.dbnames) | ||
| def test_existing_tables(dbname): |
There was a problem hiding this comment.
Maybe test_tables_exist is a more descriptive name?
| connMySql = pymysql.connect(host="neutrinos1.ific.uv.es", | ||
| user='nextreader',passwd='readonly', db=dbname) |
There was a problem hiding this comment.
Cosmetics
| connMySql = pymysql.connect(host="neutrinos1.ific.uv.es", | |
| user='nextreader',passwd='readonly', db=dbname) | |
| connMySql = pymysql.connect(host="neutrinos1.ific.uv.es", | |
| user='nextreader', passwd='readonly', db=dbname) |
gonzaponte
force-pushed
the
gonzaponte-patch-1
branch
from
b0bc591 to
4360f64
Compare
gonzaponte
force-pushed
the
gonzaponte-patch-1
branch
from
4360f64 to
fa627e2
Compare
Due to the mutability of lists, this code was modifying all the elements of the dictionary. This resulted in an attempt to download tables that don't exist in the database.