-
Notifications
You must be signed in to change notification settings - Fork 50
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
Issue #26 test cases and uri code updated #31
Conversation
pysradb/basedb.py
Outdated
self.db = sqlite3.connect(self.sqlite_file) | ||
# Originally sqlite3.connect(self.sqlite_file) | ||
self.sqlite_file = self.sqlite_file.replace('?','') | ||
self.db = sqlite3.connect('file:{}?mode=rw'.format(self.sqlite_file), uri=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to have the rw
mode and not just r
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sqlite3.connect()
has rwc
(read-write-create) mode and it gives both read-write permission to user as well as creates the file if it is not present. I changed it to rw
mode so that it does not create a new file if it does not exist and to keep the database permissions same as the original command. If we want to give only read permission I'll change it to ro
, please confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need the write/create permission. The file is downloaded from an online resource. If it does not exist, it should just throw an error. Probably it is simpler to just check os.path.isfile(input_file)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. I'll change the mode to ro
. Actually, it does throw an error in case no file is found sqlite3.OperationalError: unable to open database file
which is caught in _verify_srametadb()
function so there is no need for a separate check.
pysradb/basedb.py
Outdated
@@ -24,7 +25,9 @@ def __init__(self, sqlite_file): | |||
|
|||
def open(self): | |||
"""Open sqlite connection.""" | |||
self.db = sqlite3.connect(self.sqlite_file) | |||
# Originally sqlite3.connect(self.sqlite_file) | |||
self.sqlite_file = self.sqlite_file.replace('?','') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need to replace '?'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The '?' character was causing the input to be truncated after '?' in uri syntax. But now since you have clarified that user does not provide a wrong path, I'll remove this check.
tests/_test_sradb.py
Outdated
wrong_path = os.path.join(os.getcwd(), "data", "SRAmet") | ||
try: | ||
db = SRAdb(path) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should explicitly mention the Exception it is catching.
tests/_test_sradb.py
Outdated
db = SRAdb(path) | ||
except: | ||
pass | ||
assert os.path.isfile(wrong_path) == False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant test (as is user provided wrong path) and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thanks for clarifying. I will remove it.
I'll add other test cases then
tests/_test_sradb.py
Outdated
db = SRAdb(path) | ||
except: | ||
pass | ||
assert os.path.isfile(path) == False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant check, can be removed.
tests/_test_sradb.py
Outdated
path = os.path.join(os.getcwd(), "data", "{}".format(wrong_filename)) | ||
try: | ||
db = SRAdb(path) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires explicit Exception.
tests/_test_sradb.py
Outdated
pass | ||
try: | ||
db = SRAdb(wrongfile_path) | ||
assert False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is being tested here..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is to check if it throws an error in case a wrong filename was provided by the user and the wrong filename already existed. It should throw error sqlite3.OperationalError: no such table: metaInfo
.
However, since we are assuming that the user provides correct input then this test is also redundant.
tests/_test_sradb.py
Outdated
db = SRAdb(wrongfile_path) | ||
assert False | ||
except Exception as e: | ||
assert True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What use case is this test case handling?
Thanks @DaasDaham for your contribution! I have left inline reviews. |
Yes travis is throwing |
Hi @DaasDaham, I cannot think of an alternate to test the Thanks once again for your contribution! |
Hi @saketkc I have made all the required changes. Please have a look |
Hi @saketkc, I haven't heard from you regarding this issue, I have made the changes that you mentioned in my latest commit. Please have a look at it. |
tests/test_sradb.py
Outdated
path = 'SRAmetadb.sqlite' | ||
try: | ||
db = SRAdb(path) | ||
assert False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why assert False
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because while testing, SRAmetadb.sqlite
file won't exist in the current directory, hence if it succeeds in establishing a connection, then it would be wrong, hence the assert False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ummm, but an error would be raised before hand and hence this statement is practically unreachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, practically it shouldn't reach it.
This statement is just for edge case in case it makes a connection...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it should be removed then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I've removed it in the latest commit.
But travis is failing to build even though pytest passes all checks. It may be because of the black --check statement.
try: | ||
db = SRAdb(path) | ||
assert False | ||
except SystemExit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand which case would a SystemExit
error arise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case if a file does not exist, sqlite3.connect()
(line 27) in basedb.py will throw an Operational error which will be caught in except:
statement (line 152 in sradb.py) which will eventually lead to a sys.exit(1)
on line 157 in sradb.py. This is the SystemExit
that my function would catch.
removed assert False statement
@DaasDaham can you run |
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
===========================================
+ Coverage 24.17% 39.02% +14.84%
===========================================
Files 8 5 -3
Lines 1187 984 -203
===========================================
+ Hits 287 384 +97
+ Misses 900 600 -300
Continue to review full report at Codecov.
|
Thanks for the help! , it builds successfully now |
Thanks for your contribution @DaasDaham! Are you okay with your name being added in CONTRIBUTORS? |
Sure, thanks!. |
Hi @DaasDaham, are you okay with me adding you to the AUTHORS list? |
Tested with all existing test cases (all passed) and added new test cases.
List of changes:
_verify_srametadb(sqlite_file)
function before calling super() to ensure first that the file is right since this function also callsBASEdb
.sqlite.connect()
to use uri syntax which can be used to control details of the newly created database connection.