Make tests consistent#175
Conversation
JKutscha
commented
Aug 9, 2021
JKutscha
commented
- don't rerun pit, if we run in a timeout
- make sure we are getting the menu from the workbench window
* don't rerun pit, if we run in a timeout * make sure we are getting the menu from the workbench window
|
@LorenzoBettini or @echebbi would you have a look at these changes and merge please? |
LorenzoBettini
left a comment
LorenzoBettini
left a comment
There was a problem hiding this comment.
I have a few doubts about the code: I think a few parts should be simplified as shown in the comments. I'd also like to know more about the situations they try to fix
| pitclipseSteps.mutationIsOpened(BAR_CLASS + ".java", 7); | ||
| final long timeOutBefore = SWTBotPreferences.TIMEOUT; | ||
| try { | ||
| SWTBotPreferences.TIMEOUT = 10000; |
There was a problem hiding this comment.
Why do you need to play with SWTBot timeout in the test code?
Please don't do that!
We already increase SWTBot timeout in the Maven build and in the Eclipse launch configuration by passing something like
-Dorg.eclipse.swtbot.search.timeout=90000
There was a problem hiding this comment.
Because this timeout value is used for how long we are waiting for the pit run. Currently, the timeout is set to 5 Seconds and I turned it up to 10 seconds to give pit enough time to finish.
Should I use the search timeout instead, but this seems to be very long?
There was a problem hiding this comment.
I think that we should use the same timeout consistently and make it configurable from the outside as we do right now... @echebbi what do you think?
There was a problem hiding this comment.
I think that we should use the same timeout consistently and make it configurable from the outside as we do right now
@LorenzoBettini I'm not sure I understood you, what do you mean exactly by "make it configurable from the outside"? As you said above the timeout is already configurable through the org.eclipse.swtbot.search.timeout property.
I agree that we should keep the timeout as consistent as possible thoughout the tests and avoiding changing it programmatically. Ideally I'd actually like to have a dedicated property that defines the timeout before we stop to wait for PIT so that we can change it without impacting the timeout for UI elements; something like org.pitest.pitclipse.tests.pit.timeout, but we'll see that it in another PR.
@JKutscha it looks like you reverted this change in your latest commit and the build seems to succeed so I guess this conversation can be closed? If you do need to increase the timeout, we set it here for the Maven build:
There was a problem hiding this comment.
I think that we should use the same timeout consistently and make it configurable from the outside as we do right now
@LorenzoBettini I'm not sure I understood you, what do you mean exactly by "make it configurable from the outside"? As you said above the timeout is already configurable through the
org.eclipse.swtbot.search.timeoutproperty.
Sorry for the misunderstanding: I meant "keep it" as it is right now :)
I agree that we should keep the timeout as consistent as possible thoughout the tests and avoiding changing it programmatically. Ideally I'd actually like to have a dedicated property that defines the timeout before we stop to wait for PIT so that we can change it without impacting the timeout for UI elements; something like
org.pitest.pitclipse.tests.pit.timeout, but we'll see that it in another PR.
Yes, that should be trivial to implement. If you have time please open an issue or I'll do that ASAP
There was a problem hiding this comment.
If you have time please open an issue or I'll do that ASAP
Done, see #178
| } | ||
| } | ||
|
|
||
| public SWTBotMenu findWorkbenchMenu(final SWTWorkbenchBot bot, final String menuString) { |
There was a problem hiding this comment.
It might be the case to write a well-documenting Javadoc here, in particular, the problems it fixes
removed empty line
|
Thank you @JKutscha ! |
3 participants
