UI tests for 'Rename' and 'Extract Local Variable' refactorings.

UI tests for 'Rename' and 'Extract Local Variable' refactorings.

R=devoncarew@google.com, pquitslund@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=24980

[devoncarew, 2013-07-12 16:52:07]

lgtm

Looks good. Some thoughts:

It doesn't look like your tests or helper depend on swtbot. We'll probably want
to extract the helper code out into a common (non-swtbot) plugin. The tests
possible as well. I don't see the swtbot tests running on the buildbots anytime
soon, but I would like to get some subset of the UI tests running up there.

[scheglov, 2013-07-12 20:56:47]

PTAL

[devoncarew, 2013-07-12 22:04:38]

lgtm

Can you add a readme.txt describing the difference between src/, src-ui/, and
src-framework.? I.e. which one should be used for what.

[pquitslund, 2013-07-12 22:18:42]

LGTM % nits.  No show-stoppers but I'd really like to make sure we get
conditional waits right.

Great stuff! :)

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java
(left):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:34:
private int m_lastDragY;
Nit: why these funny/old school field names?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:52:
////////////////////////////////////////////////////////////////////////////
I think we should remove these grouping comments since they don't play nice with
our method ordering rule.

For that matter, I think this needs to be auto-formatted?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:154:
//  }
Maybe remove?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:440:
Display.getCurrent().post(event);
So I gather from this that these methods need to be called from the UI thread?
In that case, maybe we can document that and/or add an assertion so that it's
obvious when we misuse them.  For example:

    assertTrue("Events can only be sent from the UI thread",
Display.getCurrent() == null);

Alternatively, we could also be really generous and run it on the UI thread if
it's found we're not on it but that's probably going too far!

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:461:
Display.getCurrent().post(event);
Maybe a post(event) utility method?  (If you chose to validate thread access you
could do it there.)

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java
(left):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:60:
waitEventLoop(0);
What does this do?  Why wait for 0ms?  Is there some other side effect of
calling wait that you're depending on?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:85:
eventSender.keyDown(SWT.BS);
Why the BS here?  (Just curious.)

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:88:
waitEventLoop(0);
Again: why the 0 wait?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:123:
waitEventLoop(3000);
Is 3000 safe?  Can we replace this with a proper condition? That is something
like:

waitFor(new EditorActiveCondition(...));

IME timed waits are dangerous (at best you needlessly wait, at worst you don't
wait long enough).

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:132:
eventSender.keyDown(SWT.CR);
Being geeky, I wonder if there's some more literate API we could use here:

sender.keyDown(SWT.BS)
          .keyDown( 'r')
          .keyDown(SWT.CR);

On second thought...  I'm not so sure.  A thought anyway.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:151:
IAction renameAction = getEditorAction(JdtActionConstants.RENAME);
JdtActionConstants?  Is that what we want here?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:29:
* Executes the sequence of the {@link Operation}s.
How about: "Executes a sequence of {@link Operation}s."

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:39:
* Schedules the {@link Operation} execution.
 => Schedules the {@link Operation} for execution.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:49:
display.timerExec(5, new Runnable() {
Why the 5ms wait?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:52:
display.timerExec(5, this);
And again?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:78:
// wait for successful completion of failure
=> wait for successful completion or failure

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:49:
* @author scheglov_ke
Remove?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:51:
public class UiContext {
Food for thought: this class has a lot of responsibilities.  I like it as a
facade but if it gets any bigger maybe we should break it into a few classes to
hold the implementations of the misc major concerns (e.g., a finder, a selector,
a verifier).  Or not! :)

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:56:
////////////////////////////////////////////////////////////////////////////
Again: I think we want to remove these grouping comments?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:219:
public static void waitEventLoop(int time) {
In a follow-up CL, I'd like to see this replaced with a more generic
waitFor(Condition) method and *strongly* discourage unconditional waits.  I'm
happy to help with that.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:257:
private final Display m_display;
Can we "modernize" these field names?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:130:
* Selects the find occurrence of the given pattern.
How about "Selects and reveals the range associated with the given pattern" ?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:133:
int position = findOffset(pattern);
Should it be an error if position is -1?

assertThat(position).isNotEqualTo(-1);

?

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:146:
UiContext.waitEventLoop(0);
Again, this magic 0-length wait.  Looks fishy.  If we do really need it, perhaps
a new method with an intention-revealing name?

[scheglov, 2013-07-12 23:01:21]

PTAL

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java
(left):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:34:
private int m_lastDragY;
On 2013/07/12 22:18:42, pquitslund wrote:
> Nit: why these funny/old school field names? 

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:52:
////////////////////////////////////////////////////////////////////////////
On 2013/07/12 22:18:42, pquitslund wrote:
> I think we should remove these grouping comments since they don't play nice
with
> our method ordering rule.
> 
> For that matter, I think this needs to be auto-formatted?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:154:
//  }
On 2013/07/12 22:18:42, pquitslund wrote:
> Maybe remove?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:440:
Display.getCurrent().post(event);
On 2013/07/12 22:18:42, pquitslund wrote:
> So I gather from this that these methods need to be called from the UI thread?
> In that case, maybe we can document that and/or add an assertion so that it's
> obvious when we misuse them.  For example:
> 
>     assertTrue("Events can only be sent from the UI thread",
> Display.getCurrent() == null);
> 
> Alternatively, we could also be really generous and run it on the UI thread if
> it's found we're not on it but that's probably going too far!

Thank you for comment.
Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/EventSender.java:461:
Display.getCurrent().post(event);
On 2013/07/12 22:18:42, pquitslund wrote:
> Maybe a post(event) utility method?  (If you chose to validate thread access
you
> could do it there.)

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java
(left):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui.swtbot_test/src/com/google/dart/tools/ui/internal/refactoring/RenameLinkedModeTest.java:60:
waitEventLoop(0);
On 2013/07/12 22:18:42, pquitslund wrote:
> What does this do?  Why wait for 0ms?  Is there some other side effect of
> calling wait that you're depending on?

This is old code, removed now.
It was waiting 0ms to run one events loop.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:29:
* Executes the sequence of the {@link Operation}s.
On 2013/07/12 22:18:42, pquitslund wrote:
> How about: "Executes a sequence of {@link Operation}s."

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:39:
* Schedules the {@link Operation} execution.
On 2013/07/12 22:18:42, pquitslund wrote:
>  => Schedules the {@link Operation} for execution.

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:49:
display.timerExec(5, new Runnable() {
On 2013/07/12 22:18:42, pquitslund wrote:
> Why the 5ms wait?

We run the loop to check if Operation is ready to run.
We don't know when this will happen, so we have to poll.
We can later to play with this time to see if it is too small (slows down the
Eclipse) or too big (causes too long pauses).

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:52:
display.timerExec(5, this);
On 2013/07/12 22:18:42, pquitslund wrote:
> And again?

Just reschedule in case if operation is not ready to run yet, or there is a new
operation to run.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/driver/OperationExecutor.java:78:
// wait for successful completion of failure
On 2013/07/12 22:18:42, pquitslund wrote:
> => wait for successful completion or failure

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:49:
* @author scheglov_ke
On 2013/07/12 22:18:42, pquitslund wrote:
> Remove?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:51:
public class UiContext {
On 2013/07/12 22:18:42, pquitslund wrote:
> Food for thought: this class has a lot of responsibilities.  I like it as a
> facade but if it gets any bigger maybe we should break it into a few classes
to
> hold the implementations of the misc major concerns (e.g., a finder, a
selector,
> a verifier).  Or not! :)

I agree.
This class is a mess.
Mainly because it was copied from the different project.
May be worth to iterate on it and remove everything what we don't use right now.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:56:
////////////////////////////////////////////////////////////////////////////
On 2013/07/12 22:18:42, pquitslund wrote:
> Again: I think we want to remove these grouping comments?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:219:
public static void waitEventLoop(int time) {
On 2013/07/12 22:18:42, pquitslund wrote:
> In a follow-up CL, I'd like to see this replaced with a more generic
> waitFor(Condition) method and *strongly* discourage unconditional waits.  I'm
> happy to help with that.

Ah... Now I understand.
May be just bad name.
It is not supposed to be an unconditional wait, after which we blindly assume
that something is done.
This method is just a way to allow Eclipse to run UI loop for some time and then
check condition.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:257:
private final Display m_display;
On 2013/07/12 22:18:42, pquitslund wrote:
> Can we "modernize" these field names?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:130:
* Selects the find occurrence of the given pattern.
On 2013/07/12 22:18:42, pquitslund wrote:
> How about "Selects and reveals the range associated with the given pattern" ?

Done.

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:133:
int position = findOffset(pattern);
On 2013/07/12 22:18:42, pquitslund wrote:
> Should it be an error if position is -1?
> 
> assertThat(position).isNotEqualTo(-1);
> 
> ?

Already done in findOffset().

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-ui/editor/AbstractDartEditorTest.java:146:
UiContext.waitEventLoop(0);
On 2013/07/12 22:18:42, pquitslund wrote:
> Again, this magic 0-length wait.  Looks fishy.  If we do really need it,
perhaps
> a new method with an intention-revealing name?

Good idea.
Done.

[pquitslund, 2013-07-12 23:04:47]

Awesome.  LGTM.  Thanks!

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java
(right):

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:51:
public class UiContext {
On 2013/07/12 23:01:21, scheglov wrote:
> On 2013/07/12 22:18:42, pquitslund wrote:
> > Food for thought: this class has a lot of responsibilities.  I like it as a
> > facade but if it gets any bigger maybe we should break it into a few classes
> to
> > hold the implementations of the misc major concerns (e.g., a finder, a
> selector,
> > a verifier).  Or not! :)
> 
> I agree.
> This class is a mess.
> Mainly because it was copied from the different project.
> May be worth to iterate on it and remove everything what we don't use right
now.

Sounds good!

https://codereview.chromium.org/18548007/diff/4001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui_test/src-framework/com/google/dart/ui/test/util/UiContext.java:219:
public static void waitEventLoop(int time) {
On 2013/07/12 23:01:21, scheglov wrote:
> On 2013/07/12 22:18:42, pquitslund wrote:
> > In a follow-up CL, I'd like to see this replaced with a more generic
> > waitFor(Condition) method and *strongly* discourage unconditional waits. 
I'm
> > happy to help with that.
> 
> Ah... Now I understand.
> May be just bad name.
> It is not supposed to be an unconditional wait, after which we blindly assume
> that something is done.
> This method is just a way to allow Eclipse to run UI loop for some time and
then
> check condition.

Got it.  Thanks for the clarification!

[scheglov, 2013-07-12 23:07:02]

Committed patchset #3 manually as r24980 (presubmit successful).