-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
There's some style errors, see the Travis build. |
Fixed. |
FWIW, you can flake8 on your own machine before committing to avoid things like that. |
Thanks mtomwing, that will save some time in the future. |
|
||
talk_ids = db.get_talk_ids() | ||
talk_id_count = 0 | ||
while(talk_ids.next()): |
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 ain't C.
while condition:
...
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.
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.
Fixed the while loops. I will try to catch up to this Python while it opens doors.
9c82e20
to
75b3d7b
Compare
assert talk_ids.first() | ||
talk_id_record = talk_ids.record() | ||
talk_id = talk_ids.value(talk_id_record.indexOf('id')).toString() | ||
assert talk_id == '1' |
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.
Might as well use FIRST_TALK_ID
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.
Fixed.
I'm not a big fan of |
Removed FIRST_TALK_ID in favor of '1' and removed the string concatenation from os.path.join calls. |
@@ -1,2 +1,3 @@ | |||
[run] | |||
omit = freeseer/tests/* | |||
|
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.
Revert this change.
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.
reverted
187f217
to
8756aad
Compare
''') | ||
|
||
def __drop_presentation(self): | ||
QtSql.QSqlQuery('DROP TABLE presentations') |
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.
These functions definitely do not go here as they're only used by tests. I'd also question whether they're actually required at all?
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.
Migrated to the testing function. I thought of a way to get them out of database.py
yesterday but was not able to get back to the keyboard.
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.
If there are end users running older versions of the client then it could be useful to test that their databases upgrade correctly when they get a newer version of freeseer? Although I am not sure how many people actually run freeseer with a very old version like 2x
|
||
talk_id = db.get_talk_between_dates(presentation.event, presentation.room, | ||
time_before_presentation, time_after_presentation) | ||
assert talk_id == '1' |
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 test can be improved by inserting more presentations, to make sure it actually filters presentations.
edit: The below test kinda tests that already.
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 added another test. I also added another presentation object to this test. I have added a fixme comment to the get_talk_between_dates function since it only returns one presentation id (if there are multiple presentations on the same day which one has its id returned?)
…o fixtures, need to give them better names, refactored helper function name, added logging, added more tests.)
…d to presentation for comparing records to presentations. Started refactoring failure and presentation references.
@dideler |
@dideler added the |
@dideler I will take a look again on Friday+Saturday for refactoring. I know for sure that the presentation fixtures will need to be renamed. There may also be some other little points that can be fixed up as well to be cleaner/clearer. |
|
||
talk_ids = db.get_talk_ids() | ||
expected_id = 1 | ||
while(talk_ids.next()): |
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.
No parentheses for while
.
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.
fixed.
Thanks for the updates. I'll try to do some more review and leave comments before Friday/Saturday. On a sidenote, this PR is getting huge with the amount of conversations, which makes it slow to load and linking to conversations doesn't work anymore (at least not for me). Might be useful to create a new PR and then reference this one. |
edit: Not certain how to associate the PR with the other issue though. Ah I made an Issue instead of a PR, oops. The PR creation page doesn't mention creating another PR, only to view the current one associated with the branch. Guess it can be cloned and a PR created from that. edit3: #671 created |
Discussion will continue in #671. P.S. @wigglier the easiest way to open a replacement PR is to close the existing PR first, then open a new one. You shouldn't have to create a new branch in the process. |
Updates to
tests/framework/test_database.py
andframework/database.py
.test_database.py
unittest
references withpytest
referencesRelated gh484