Closed vnlitvinov closed 4 years ago
@vnlitvinov thanks for caching that up.
could you add tests for that to omniscidb/tests/
?
would be good to have a test for the 2 cases, when the database name is the same of the current_database and for a different name (maybe you can use the omnisci
database or you will need to create a temporary database for the test.
thanks!
@abykovsk could you please take a stab at making required tests?
@abykovsk could you please take a stab at making required tests?
There should be a test that failed before merging #1964, so this never broke. There is this tests but since name == None
the breaking code is never called. I guess you can add a pytest parametrization to that test, so it's called with name == None
but also to a different database name to the current want.
Hello @vnlitvinov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
would be good to have a test for the 2 cases, when the database name is the same of the current_database and for a different name
I have added a separate test (the case where database name is same as current behaves exactly the same as with database name being None
and is already handled by test_database_layer
).
I tried parametrizing it first, but felt it would end up too complex (and it would actually test two unrelated things which isn't good for a unit test), so I wrote a new test instead.
Anything left for me to do to make this accepted?
looks ok, but tests are failing
I know, we're looking at it. The test should probably be enabled only for SQL-like backends.
@jreback ping :)
@vnlitvinov omnisci is currently not being tested, so all merging on hold
What's the reason for this? Can we help with that being fixed?
@vnlitvinov can you merge master, and check if the CI is still green please? CI is working for omnisci, we can move forward with this now
@datapythonista I've rebased instead for a cleaner history.
The CI failures look strange and unrelated to our changes... can I do something to get this merged?
@vnlitvinov we need a test that fails on master and is fixed by this change
@jreback it would be nice to see this requirement a bit earlier than in almost 3 months... in a PR fixing a typo (original diff was one line).
Am I reading this right that you want me to make a separate PR (with a failing test) which would turn master CI red, and only then fix that test? All for 1 line (and two words) worth of a change?..
Also note there is a test added that would fail on master if ran now: https://github.com/ibis-project/ibis/pull/2106/files#diff-2268971ed7d3b780e597e36b7a466045R203
i commented quite a while ago on this
one line or a million it doesn’t matter
if something is claimed to be fixed then it needs a test
often the test are way longer than the patch
i want a test included with this PR one that fails in master
i want a test included with this PR one that fails in master
https://github.com/ibis-project/ibis/pull/2106/files#diff-2268971ed7d3b780e597e36b7a466045R203
changes look ok. rebase on master and some comments, ping on green.
@jreback I think we addressed all your comments (so I marked them as resolved), and after merge with master
CI is finally green.
thanks @vnlitvinov
1964 changed the API for
OmniSciDBClient
but didn't change all usages ofdevice=
parameter when constructing a client. In my case aconnection.database()
call was failing.