-
Notifications
You must be signed in to change notification settings - Fork 110
WIP: #630: Create tests for talkeditor.py following PR #605 #631
base: master
Are you sure you want to change the base?
Conversation
c8446ae
to
6c3c107
Compare
6c3c107
to
304ecab
Compare
# This was just a test to see if it could be done -- the "exec_()" method is a bit tricky. | ||
# This will probably be more useful in test_show_save_prompt() | ||
#QTimer.singleShot(0, self.talk_editor.newTalkWidget.cancelButton, QtCore.SLOT('click()')) | ||
#QtTest.QTest.mouseClick(self.talk_editor.commandButtons.addButton, Qt.Qt.LeftButton) |
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.
Segfault is because you have an invalid entry for
Qt.Qt.LeftButton
It should be
Qt.LeftButton.
Note the double Qt.Qt statement.
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.
Mmkay. I think I might have been basing that on the function right above it, "test_help_menu_about", which has the line QtTest.QTest.mouseClick(self.talk_editor.aboutDialog.closeButton, Qt.Qt.LeftButton)
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.
That function's wrong too so I'd fix it. Not sure why it didn't fail horribly.
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 tried removing the extra Qt.
from my test funciton. The test suite completed, but it said that 1 test failed, with the message AttributeError: 'module' object has no attribute 'LeftButton'
When I removed the extra Qt.
from both functions, I got almost the same thing, except this time, 2 tests failed (both with the same error).
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.
Ok I did some more investigating and here's what I found. It's failing specifically with buttons from the commandButtons widget and half of the buttons work and the other half causes segmentation faults.
importButton - works exportButton - fails addButton - fails removeButton - works removeAllButton - fails searchButton - works
What do all the buttons that cause segfaults have in common? They all open up a new dialog box in the UI. I think this is the key issue. If you click import, remove, and search buttons those all do not cause segfaults and they also don't open a new dialog box inside the UI.
So the issue seems to have to do with the fact that a new dialog box opens. I'm guessing similar to how we have to unload FreeseerApp with the delete command, we need to do something with the new dialog windows that open otherwise it causes a segfault when we try to exit the test.
At this point I'm not sure what the right solution is but at least we know exactly what the cause is now.
Edit: More details
In regards to the Add Button specifically it opens a newTalkWidget.
I found from https://groups.google.com/forum/#!topic/python_inside_maya/Eyou5lBpwZ0
We're opening the newTalkWidget wtih exec_() which is blocks the main application. Seems to be not very good for testing purposes. I changed exec_() call to use this instead:
self.d.setModal(True) self.d.show()
and at least for the Add Button case it doesn't cause the segfault anymore (Note: in my test it was very quick hack and actually breaks the new talk widget so don't use it as an immediate solution). We'll likely need to refactor that part of the code. I'm not sure if the other buttons also use this exec_() thing but if they do that's likely why they also segfault.
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.
Ben at this point I think it's probably more worth your time spent looking into testing anything else you can test rather than stay blocked on this addButton.
Edit: unless you want to refactor the code so that it doesn't segfault in testing.
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 think I'd like to refactor the code. A big part of my changes to the code was to add a new window warning the user of unsaved changes, and that uses the exec_() method. Dennis Ideler suggested changing it to setModal(True) and show(), but when I tried that, it turned out strange, as outlined in this comment (#605 (comment)). It might be a good idea for me to figure out exactly what went wrong.
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 looked at your #605 comment and was wondering how you implemented it originally?
If you did what I did to quickly test and just switched out exec_() for
d.setModal(True) d.show()
Then the buttons will not really do anything because show() is a non-blocking function. So you cannot evaluate an if statement immediately after show(). The program will continue to proceed since it's non-blocking thus whatever variable you are waiting on for user input will not have been processed yet.
You need to create a Qt SIGNAL/SLOT to connect the accept and cancel button clicks back to a function which will process the user input that you are waiting for after the user has provided their input.
Edit: Here's a few links on how Qt Signals and Slots work
http://qt-project.org/doc/qt-4.8/signalsandslots.html
http://pyqt.sourceforge.net/Docs/PyQt4/new_style_signals_slots.html
I like reading the original Qt docs because it provides better details on how things work. Once you understand that the PyQt4 link will explain how to implement it in Python. You could also look at any of our Qt Frontend code files and copy that too since we use SIGNALS and SLOTS a lot in Freeseer so there's many places you can copy examples from.
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 notice that, in some cases (such as NewTalkWidget.py and ImportTalkWidget.py) the layout of a particular feature is placed in a separate file. Would it be objectionable for me to similarly place some of the code for my save prompt in a separate file (e.g. "SavePromptWidget.py")?
The reason I didn't do this in the first place was because I was expecting it to be a fairly simple dialog window, but if I want to make it easier to test and still do what I want it to do, it's looking more and more like I might have to make the widget detailed enough to justify its own file. Thoughts, @zxiiro , @dideler ?
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 reason for splitting files is reusability. If you think someone might need to reuse your same widget in another file then make it a new separate file so that they can easily include it without pulling all of your code that they don't need.
What you have to ensure when you create it this way is that your separate file only includes View elements and does not include any logic at all since logic will make it less easy for someone to reuse the widget. Typically talkeditor.py is the logic file which glue's everything together while all the *Widget.py files only contain logicless classes that are easily importable.
ba94540
to
e3957ee
Compare
@@ -98,15 +100,26 @@ def __init__(self, config, db): | |||
self.currentTalkIndex = QPersistentModelIndex() | |||
|
|||
# Prompt user to "Continue Editing", "Discard Changes" or "Save Changes" | |||
# Can't be a QMessageBox anymore. Must be a QDialog? Put in separate 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.
Just curious, why is this?
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.
QMessageBox is designed for very simple, common features with default names (e.g. "OK", "Yes", "No") -- as you mentioned in PR #605, we want to avoid vague button labels like "Yes" and "No". Customizing titles can be done in QMessageBox, and indeed, that is what I originally did for the save prompt in the talk editor. (Note, however, that even this is listed under "Advanced Usage" in the Qt documentation: http://qt-project.org/doc/qt-4.8/qmessagebox.html#advanced-usage ).
In addition, QMessageBox doesn't lend itself well to testing. In particular, using the exec_()
method blocks any further instructions from being carried out until the message box receives input. There are ways around this (see: http://stackoverflow.com/questions/9518484/test-modal-dialog-with-qt-test) but they are a tad inelegant. I tried using QTimer.singleShot()
to do some tests for methods that used exec()
, and I was able to run the tests in test_talkeditor.py
just fine, but kept getting a segfault error whenever I ran the whole test suite.
As per @zxiiro 's recommendation (#631 (comment)), rather than spend too much more time trying to figure out how to test methods that use exec_()
, I am refactoring the code in the talk editor. This means using setModal()
and show()
instead of exec_()
.
However, as @zxiiro points out (#631 (comment)) it's not as easy as simply replacing every instance of exec_()
and leaving the rest of the code the same. The short version is, I'm going to have to use SIGNALS and SLOTS to make the buttons do what I want them to do.
The problem is: in QMessageBox
, the method addButton()
doesn't think in terms of signals and slots; it thinks in terms of standardized button roles (e.g. "rejectRole" = Cancel, "acceptRole" = Yes, and so on). You can't just add a button and then decide what you want to do with it later: you have to choose a role immediately. This is in contrast with, for example, the NewTalkWidget, where the buttons and layout are decided in NewTalkWidget.py, but the actions of those buttons is decided in talkeditor.py. QMessageBox seems to be designed with the idea of using the exec_()
method a lot: most of the examples in the documentation use exec_()
.
It would be possible to add non-functional buttons and then add the functionality later if I put some kind of layout in the QMessageBox, and then used addWidget to add the buttons to the layout. But it seems to me that if I'm going to add a layout, I might as well just use QDialog rather than QMessageBox. (Which makes sense, since QMessageBox inherits QDialog anyway).
Re: Separate file -- I think a window that asks the user whether they want to save their changes, discard their changes, or continue editing, is a sufficiently reusable feature to put in a separate file. It seems like the kind of feature for which there will be other, unforeseen circumstances under which it might be useful. My hope is that, ultimately, this will make the code a bit more elegant.
I can think of a couple ways to add functionality to the window. The issue is this. There are several cases where the user might need to be prompted to save/discard unsaved details to a talk: they might be (a) selecting a new talk, (b) exiting the talk editor, or (c) creating a new talk. In each case, the buttons "Save Changes", "Discard Changes" and "Continue Editing" should do slightly different things. I have a couple ideas for solutions, each of which seems a little odd to me. I wanted to get some idea whether you had a preference either way: (1) Have a separate SavePromptWidget for each of the 3 cases, and write a function for each of the 3 buttons, up to 9 functions in total. For example, closing the window would bring up a widget called something like (2) Have only one SavePromptWidget -- however, rewrite the functions for "Save Changes", "Discard Changes" and "Continue Editing" depending on which function is called. For example, there might be some code in the
... and there would be some code in
Similarly, Some of the syntax above might be a little wrong, but in principle I know it's possible to do this kind of thing in Python. I kind of like solution 2, probably because I write a lot of Lisp programs in my spare time, and it's not uncommon in Lisp to be this cavalier about treating functions like data. I started writing it this way, but before I got too far, I thought I should consult the mentors and the PEP8 guide. The PEP8 guide doesn't have anything nice to say about using lambda, and a quick search for the word "lambda" in the repository only brings up a couple of fairly simple, probably very routine examples, so I was hesitant to program it in such an unconventional way. |
What first comes to mind is polymorphism. That is, having an abstract base class In your second suggestion, you're monkeypatching, which isn't as nice of a solution IMO (code becomes more confusing). But whichever way works best in this situation. I would try monkeypatching last, unless there's good reason to try it first. P.S. In most cases, partials should be preferred over lambdas. P.P.S. Please use code blocks so your multi-line code examples are easier to read. |
e3957ee
to
a733e1e
Compare
It's a little hacky-looking right now, but it kinda sorta-ish works. I will definitely want to clean it up a lot. |
@@ -339,7 +399,7 @@ def click_add_button(self): | |||
self.talk_selected(self.currentTalkIndex) | |||
self.show_new_talk_popup() | |||
else: | |||
log.info("Continue editing row %d", self.currentTalkIndex.row()) | |||
log.info("Continue editing row %d", self.currentTalkIndex.row())''' |
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.
While you may think it's handy, using docstring comments to comment out code is bad. This will result in api documentation including your commented out code.
I'd recommend using # to comment out blocks of code, good code editors can do this with a click of a button.
a733e1e
to
631c0da
Compare
9d96145
to
5111b38
Compare
allFields = [self.titleLineEdit, self.presenterLineEdit, self.categoryLineEdit, | ||
self.eventLineEdit, self.roomLineEdit, self.dateEdit, self.startTimeEdit, | ||
self.endTimeEdit, self.descriptionTextEdit] | ||
for field in allFields: |
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.
Since allFields
is only created for the for-loop, the temporary variable can be skipped by iterating directly over the list.
for field in [self.titleLineEdit, ...]:
But because the list is so long, this won't work well with our max line length limit of 120 chars.
There's a lot of repeated patterns in the list elements, so we can take advantage of that to shorten the list.
for field in ['titleLineEdit', ...]:
getattr(self, field).setEnabled(bool)
Still some repeated patterns that we can move out of the list.
for field in ['titleLine', 'presenterLine', 'categoryLine', 'eventLine', 'roomLine', 'date', 'startTime', 'endTime', 'descriptionText']:
getattr(self, field + 'Edit').setEnabled(bool)
Unfortunately it's still over the max length limit :(
But we can still shave off a few characters.
for field in 'titleLine presenterLine categoryLine eventLine roomLine date startTime endTime descriptionText'.split():
getattr(self, field + 'Edit').setEnabled(bool)
That's under 120 chars, but indented it won't be. At this point it can be made into a multi-line string by triple-quoting it, but then you might as well keep it as a multi-line list.
Anyway, allFields
should be all_fields
according to our styleguide, but it might as well be simplified to fields
.
…t, and click_talk
5111b38
to
1023f86
Compare
Currently, I have (1) a refactoring that basically does what it claims to do, and (2) a couple tests. I don't think it would be fruitful to try adding a bunch of new goals to this PR at this point. If what I've done so far is basically okay, I'd like to clean it up in response to any feedback. I won't be offended, of course, if it's just not suitable to be merged. I'd just sleep better knowing that this PR is reaching some kind of conclusion. |
@benbuckley I agree that it'd be better to wrap up this PR by cleaning up what works and removing any unfinished stuff. Whatever didn't make it in can always be added in the future by someone. I don't have time tonight to review, but I'll try to review tomorrow. |
Trying to exit the talk editor while saving changes in the prompt no longer works. Edit: Actually, saving changes seems to be buggy in all three cases (adding a talk, exiting, or selecting a different talk). |
It appears to work for me, but maybe I'm missing something. Can you be more specific? The only bug I can find is the one already mentioned in issue #612 . |
I was experiencing issues like trying to exit the editor, saving changes in the prompt, but the editor not exiting. But good news -- after doing a |
|
||
self.bottomButtonLayout = QHBoxLayout() | ||
|
||
self.label = QLabel("The talk you were editing has unsaved changes.") |
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.
Be consistent with singe/double quotes. Double quotes are being used for this QLabel but single quotes for the QPushButtons below.
I was somehow able to reproduce the issues I mentioned earlier, and this time I haven't switched branches in between. I'll try to figure out the steps and post them. Edit: having difficulty figuring out the steps needed to reproduce, but in the meantime, here's the log from when I couldn't close the editor and the prompt kept popping up when trying to save the changes.
Notice the -1 row. |
Figured it out, here's a video of how to reproduce: https://www.youtube.com/watch?v=0LIWRWYafnA Trying to edit a talk via the table and then via the talk details seems to cause issues. Using the same technique for editing (double clicking the cell but then editing via talk details) also causes issues for the other type of unsaved changes prompts (i.e. add new talk, switch talk). Give it a try. IMO, we should only have one way to edit a talk. Two ways is confusing and error-prone, so I'm in favour of disabling editing from the table. |
(I'm still here, but I'm in the middle of transferring all my stuff to a new laptop.) |
Hey Ben, good to know. Good luck with the transfer. |
Now that PR #605 has been merged, I would like to add some tests for the new functions I created. I will have a better idea of what steps this requires as I start experimenting, but I thought I should create this PR as soon as possible. Ultimately, this should close issue #630 .
Oct. 20, 2014: After some research, I have a slightly better idea of what I need to accomplish. These tasks might change as more information comes along -- I haven't even learned about QtTest yet.
Oct 29, 2014: After further thinking, it might not be worth it to focus on upgrading everything to Pytest just yet. For the time being, I mostly just want to get some tests done. So, I'm putting the Pytest goals on hiatus.