Friday, May 31, 2013

Are They Yaks or Are They Shiny? Ooh Shiny!

‹prev | My Chain | next›

The scariest remaining work in the Dart version of the ICE Code Editor probably resides in the button toolbar that will reside at the top of the screen. The “Hide Code” / “Show Code” button toggles are pretty easy, but the “Update” button could be tricky. Or it could be really, really easy. Either way, it scares me, which is a pretty good indication that I need to focus there tonight.

The “Update” button itself updates the preview layer to reflect any code changes:



Since that has to push the changes into an iframe, I am a little concerned that my test will get overly asynchronous. Dart does async tests pretty well, but I have been writing some wonderfully procedural functional tests of late. I am unsure how these will fit in.

The update button also includes a checkbox toggle to enable/disable auto-updates of the preview layer after the programmer has stopped typing. I am really unsure how that is going to work in my tests since it is impossible to create keyboard events in Dart and since the delay is 2 seconds — I begrudge tenths of seconds that get added to the tests.

Anyhow, I start by simply adding the buttons:
class Full {
  // ...
  _attachToolbar() {
    var toolbar = new Element.html('<div class=ice-toolbar>');

    toolbar.children
      ..add(_updateButton)
      ..add(_hideCodeButton)
      ..add(_mainMenuButton);

    el.children.add(toolbar);
  }

  get _updateButton {
    return new Element.html('''
        <button>
           <input type="checkbox"/> Update
         </button>'''
      );
  }

  get _hideCodeButton {
    return new Element.html('<button>Hide Code</button>');
  }
  // ...
}
Easy enough: I add two new buttons to the main toolbar, building both from HTML. I make use of Dart getters to cut down on parenthesitis. I also use multi-line strings when the update button threatens to get too wide. Ah, Dart, there are so many reasons why I love you. Except...

That simple change breaks 14 tests. What?

It turns out that many of my functional tests queried for the first <input> element in the document when trying to fill out dialog forms. Unfortunately, this change introduces another input—the checkbox <input> that will eventually toggle code auto updates.

The simple fix is easy enough. In each test, I query for the first text input field:
    test("can open the rename dialog", (){
      helpers.click('button', text: '☰');

      helpers.click('li', text: 'New');
      query('input[type=text]').value = 'My New Project';
      helpers.click('button', text: 'Save');
      // ...
    });
Ooh! I see an opportunity for another test helper here. I don't think I am going to get to the Update button tonight (unless in tonight's #pairwithme), but I think I can improve that helper to read:
    test("can open the rename dialog", (){
      helpers.click('button', text: '☰');

      helpers.click('li', text: 'New');
      helpers.typeIn('My New Project');
      helpers.click('button', text: 'Save');
      // ...
    });
If I can make it work, that would be very nice. The entire test setup could then be expressed in terms of “click on the button with this text” or “type in that value”. I think that worth the detour. Thanks to document.activeElement, which points to the currently active element in a web page (or document.body if nothing is active), I can write that helper as:
typeIn(String text)=>  document.activeElement.value = text;
In order for that to work, the various input fields in the UI have to be active, but we have been painstakingly doing just that over this past week (for a better UI experience).

In the end, I convert 14 instances of the query-element-set-value approach to the new helpers.typeIn() approach. The tests remain nice and snappy and are even more readable. That seems like a good win for the night—even if it was not the win I set out to get.


Day #768

Thursday, May 30, 2013

Life with Many Small Code Files in Dart

‹prev | My Chain | next›

Now that I have my one-class-per-dialog branch merged into master, I am ready to continue “regular” development on the Dart version of the ICE Code Editor. Hopefully I can make good use of the newly separate code files.

I start with the hit-enter-to-save feature. More specifically, I start with a test. And since I have a new_project_dialog_test.dart file, it is easy to figure out where to put that test. At the bottom of the file, I add:
    test("hitting the enter key saves", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');

      query('input').value = 'My New Project';
      document.activeElement.dispatchEvent(
        new KeyboardEvent(
          'keyup',
          keyIdentifier: new String.fromCharCode(27)
        )
      );

      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Open');

      expect(
        queryAll('div'),
        helpers.elementsContain('My New Project')
      );
    });
This is a test of the functional variety. It clicks the menu button, selects the “New” menu item and types in “My New Project” for the project name. Then, I fake a key event (because Dart cannot do the real thing) for Enter. Finally, I test that a project was created by inspecting the “Open” menu.

In addition to being something of hack, that keyboard event sticks out among my otherwise pristine DOM test helpers. I move it and a couple of thoughtful shortcuts into my helers.dart:
hitEnter()=> type(13);
hitEscape()=> type(27);

type(int charCode) {
  document.activeElement.dispatchEvent(
    new KeyboardEvent(
      'keyup',
      keyIdentifier: new String.fromCharCode(charCode)
    )
  );
}
This means that my test can now be written as:
    test("hitting the enter key saves", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');

      query('input').value = 'My New Project';
      helpers.hitEnter();

      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Open');

      expect(
        queryAll('div'),
        helpers.elementsContain('My New Project')
      );
    });
That's much nicer!

Of course, my test fails, but it is easy enough to get passing by adding some code. Thanks to my one-class-per-dialog refactoring, finding the class is much easier now. In new_project_dialog.dart, I listen to the onKeyUp event stream:
part of ice;

class NewProjectDialog {
  // ...
  open() {
    // ...
    dialog.query('button')
      ..onClick.listen((e)=> _create());

    dialog.query('input')
      ..onKeyUp.listen((e){if (_isEnterKey(e)) _create();});
  }
  // ...
}
Next, I need to make sure that creating a new project opens the project immediately. It is kind of an obvious feature, but one that I have yet to drive with tests. The test:
    test("creating a new project opens it immediately", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');
      query('input').value = 'My Project';
      helpers.click('button', text: 'Save');
      editor.content = 'asdf';
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Save');

      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');
      query('input').value = 'My New Project';
      helpers.click('button', text: 'Save');

      expect(
        editor.content,
        equals('')
      );
    });
Exercising the menus in the full-screen editor, I create a project, edit the content, save it and then create a new project. At this point the content should be blank, but this test tells me that:
FAIL: New Project Dialog creating a new project opens it immediately
  Expected: ''
       but: was 'asdf'.
That is a pretty easy test to get passing—I only need set the content of the editor to the empty string immediately after creating a project:
part of ice;

class NewProjectDialog {
  // ...
  _create() {
    var title = query('.ice-dialog').query('input').value;
    if(store.containsKey(title)) {
      // prevent user from over-writing an existing project...
    }
    else {
      store[title] = {};
      ice.content = '';

      _hideDialog();
    }
  }
}
With that, the project is up to 59 passing tests.

Before calling it a night, I take one more advantage of the newly separate classes and tests. I add some TODOs to the new_project_dialog_test.dart test file:
part of ice_test;

new_project_dialog_tests(){
  group("New Project Dialog", (){
    test("new project input field has focus", (){ /* ... */}
    test("cannot have a duplicate name", () { /* ... */}
    test("can be named", (){ /* ... */}
    test("clicking the new menu item closes the main menu", (){ /* ... */}
    test("the escape key closes the new project dialog", (){ /* ... */}
    test("hitting the enter key saves", (){ /* ... */}
    test("creating a new project opens it immediately", (){ /* ... */}
  });
  // TODO: templates
  // TODO: blank name behavior
}
That makes it easy to pick up with a #pairwithme partner or the next time I have to work on something. I am so glad that multiple code files are cheap in Dart. I am even more glad that I finally put that feature to good use in this project.


Day #767

Wednesday, May 29, 2013

Many, Many Dart Test Parts

‹prev | My Chain | next›

The test suite in the Dart version of the ICE Code Editor is a slave to the JavaScript included in it. And it is making my test suite messier than I would like. Unfortunately, I do not think that I have much choice in the matter.

Actually, I do have a choice. I am intentionally hiding the JavaScript nature of ICE from developers. I want them to be able to create an instance of ICE with just a couple lines of Dart and no icky JavaScript:
<head>
  <script type="application/dart">
import 'package:ice_code_editor/ice.dart' as ICE;

main()=> new ICE.Full();
  </script>
</head>
Since there is JavaScript in ICE (the actual code editor is ACE code editor), this means that I have to dynamically add <script> tags to pull in the JavaScript.

This works quite well in the application code, but is a mess in the test suite. I have code that prevents multiple <script> tags from being added for ACE (yes, it is a bit confusing with ICE and ACE—I chose ICE before I had even heard of ACE). But the preventative duplicate <script> tag is the only nod that I give to testing in the application code.

It is never a good idea to optimize for the testing environment, so I do not intend to do so today. But today may be the day that the current approach gets out of control. Currently, the test suite looks like:
library ice_test;

// import libraries for testing here...

part 'editor_test.dart';
part 'store_test.dart';
part 'gzip_test.dart';
part 'full_test.dart';

main(){
  editor_tests();
  store_tests();
  gzip_tests();
  full_tests();
}
I want to extract the dialog tests out of full_test.dart, which is currently 561 lines of code and nigh impossible to search for the group that I want.

I pull the copy-related tests out into a copy_dialog_test.dart file:
part of ice_test;

copy_dialog_tests() {
  group("Copy Dialog", (){
    var editor;

    setUp(()=> editor = new Full(enable_javascript_mode: false));
    tearDown(() {
      document.query('#ice').remove();
      new Store().clear();
    });

    test("can open copy dialog", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Make a Copy');

      expect(
        queryAll('button'),
        helpers.elementsContain('Save')
      );
    });
    // ...
  });
}
As the opening declaration indicates, I make this part of the whole test suite rather than a smaller library. I found it best to keep all of the code in the same execution isolate—again mainly due to js-interop concerns. Since this is part of the whole ice_test library, I need to mark it (and the other dialog tests) as a part, and run the test function:
library ice_test;

// imports...
part 'editor_test.dart';
part 'store_test.dart';
part 'gzip_test.dart';

part 'full_test.dart';
part 'full/copy_dialog_test.dart';
part 'full/share_dialog_test.dart';
part 'full/open_dialog_test.dart';
part 'full/new_project_dialog_test.dart';
part 'full/rename_dialog_test.dart';
part 'full/save_dialog_test.dart';
part 'full/remove_dialog_test.dart';

main(){
  editor_tests();
  store_tests();
  gzip_tests();

  full_tests();
  copy_dialog_tests();
  share_dialog_tests();
  open_dialog_tests();
  new_project_dialog_tests();
  rename_dialog_tests();
  save_dialog_tests();
  remove_dialog_tests();
}
I am not thrilled at the large number of parts. That said, it is not as horrible as I had feared. All 50+ tests still pass. The complexity of the test organization is handled in one file—albeit in two places. Even if I coud split this test suite out into libraries, the full tests would likely be in the same library and it would not end up much cleaner.

Overall, I think this approach is probably worth the parts hassle (and, really, it's not much of a hassle). Instantly knowing exactly where all copy-dialog tests (or any other dialog tests) are located is going to help immensely.


Day #766

Tuesday, May 28, 2013

Git Rebase and Dart

‹prev | My Chain | next›

I think that I have settled on an approach to the menu system in the Dart version of the ICE Code Editor. I need to convert the rest of the dialogs in the dialog-classes branch over to the strategy pattern before I am ready to merge the changes back into master, but first, I have a good problem to have™. Both the master branch and the dialog-classes branch have undergone significant work in the same area over the past few days.

The best way to fix this, I think is to git rebase the dialog-classes branch back onto master. Currently, they diverged back at commit b0ace77:
➜  ice-code-editor git:(dialog-classes) git lola         
* 2ca6894 (HEAD, origin/dialog-classes, dialog-classes) Move MenuItem class into its own file
* c49f3d4 Try OpenDialog as a strategy
* 5512abb Private methods to contruct menu objects
* 0275d60 Rename the Projects menu as Open
* b10ae98 Rename projects dialog as open dialog
* 630b839 Extract sub-menus and dialogs out into classes
* 1d91d06 Account for renamed in newly rebased code
* bdf1554 Internal script to generate docs
* 1f85bdc Extract ProjectsDialog into its own file
* 0b8ecb2 Pull the project menu out into a class
| * 38ab658 (origin/master, master) rename feature: the rename input filed has focus
| * ba06eac get started on the rename feature
| * 40ffbb9 Improve the make-a-copy naming
|/  
*   b0ace77 Merge pull request #3 from gempesaw/makeACopy
|\  
| * d11c704 Add automatic focus and pre-populate makeCopy menu item
| * fbf286d Fix focus for share menu item
|/  
* 3e4bdcc Markdown typo
* da53f79 Implement the Make Copy feature
...
(git lola is an alias for log --graph --decorate --pretty=oneline --abbrev-commit --all)

I need to take the 0b8ecb2 Pull the project menu out into a class commit and make it so that git thinks of it as having been made after 38ab658 (origin/master, master) rename feature: the rename input filed has focus, not the current b0ace77 branch point. This is what git rebase does.

I am going to get git conflict from this, but that will either happen now or when I merge the branch back into master. I'd rather get it out of the way now.

So, I start:
➜  ice-code-editor git:(dialog-classes) git rebase master
First, rewinding head to replay your work on top of it...
Applying: Pull the project menu out into a class
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging lib/full.dart
CONFLICT (content): Merge conflict in lib/full.dart
Failed to merge in the changes.
Patch failed at 0001 Pull the project menu out into a class

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To check out the original branch and stop rebasing run "git rebase --abort".
Yup.

If I take a look at the conflicting lib/full.dart, I find pretty much what I expected. The conflict is due to removing code in the branch from a section of lib/full.dart that was under active development:
class Full {
  // lots of code that doesn't conflict...
<<<<<<< HEAD
  Element get _projectsMenuItem { /* ... */ }
  _openProjectsMenu() { /* ... */ }
  _openProject(title) { /* ... */ }
  Element get _renameMenuItem { /* ... */ }
  _openRenameDialog(){ /* ... */ }
  _renameProjectAs(String projectName){ /* ... */ }
  String get _currentProjectName{ /* ... */ }
=======
>>>>>>> Pull the project menu out into a class
  // even more code that doesn't conflict...
}
I don't think that I made any changes in master to the project menu code and I definitely removed it from the dialog class branch. I think the conflict probably arose because of the rename menu code, which definitely is new in master. So I remove the projects code and leave the rename code in place. I then resume normal rebase operations:
➜  ice-code-editor git:(38ab658) ✗ git add lib/full.dart
➜  ice-code-editor git:(38ab658) ✗ git rebase --continue
Applying: Pull the project menu out into a class
Applying: Extract ProjectsDialog into its own file
Applying: Internal script to generate docs
Applying: Account for renamed in newly rebased code
...
Nothing is ever lost in git, so there is not much danger in removing that projects code. I also have an excellent test suite that has been driving all of my functionality so I feel even safer. In fact, I work through a few more simlilar conflicts, run my test suite and find:
Exception: Class 'Full' has no instance getter '_store@0x4729ef0'.

NoSuchMethodError : method not found: '_store@0x4729ef0'
Receiver: Instance of 'Full'
Arguments: [] 
I get a bunch of errors like that. Thankfully they are all fixed by converting the store instance variable from private (indicated with a leading underscore) to a public instance variable in some of the code that came from master. With that, I have my entire test suite passing again:
All 52 tests passed. 
unittest-suite-success
I spent the rest of the evening converting any straggler dialogs to the new class-based approach and even adding new ones. I will merge back into master tomorrow. I can sleep on it safe in the knowledge that, even if any fixes are added to master, my dialog-classes branch is close enough that a merge should be a quick affair.



Day #765

Monday, May 27, 2013

Templates, Strategies, Factories and Dart

‹prev | My Chain | next›

I don't remember where I saw it, but I seem to recall that windowing systems are one of the few times in which regular inheritance actually works. Well, the dialog classes in the Dart version of the ICE Code Editor are kinda like a windowing system. Maybe regular inheritance will work with them...

All of the dialogs classes follow a similar structure. The constructor accepts an instance of the full-screen version of the editor and sets the same three instance variables. Each dialog has a menu item getter named el. This el is not the DOM element that contains the dialog, rather it is the menu item element that is inserted in the main menu. After that, there is a method that opens the dialog and one or more subsequent methods that perform an action for the dialog.

The OpenDialog class, which provides a dialog for opening existing projects, looks like:
class OpenDialog {
  var parent, ice, store;

  OpenDialog(Full full) {
    parent = full.el;
    ice = full.ice;
    store = full.store;
  }

  Element get el {
    return new Element.html('<li>Open</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> _openProjectsMenu());
  }

  _openProjectsMenu() { /* ... */ }

  _openProject(title) { /* ... */ }
}
OK, maybe this isn't the prototypical GUI windowing system, but it still seems worth exploring inheritance.

I can extract the common elements into a base class:
class Dialog {
  var parent, ice, store;

  Dialog(Full full) {
    parent = full.el;
    ice = full.ice;
    store = full.store;
  }

  Element get el {
    return new Element.html('<li>$menuLabel</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> open());
  }

  String get menuLabel;
  void open();
}
This forces me to define the menuLabel and open() methods in any of the sub-classes:
class OpenDialog extends Dialog {
  OpenDialog(full): super(full);

  get menuLabel => "Open";

  open() { /* ... */ }

  _openProject(title) { /* ... */ }
}
This saves me a bit of work and provides a nice template for all of the dialog classes in ICE. But the template pattern of inheritance is not the end-all be-all of object oriented programming.

One of the problems with this approach is that these dialogs are performing two functions: the menu item and the subsequent dialog that opens. An alternate approach would be to create a MenuItem class:
class MenuItem {
  var dialog;
  MenuItem(this.dialog);
  Element get el {
    return new Element.html('<li>${dialog.name}</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> dialog.open());
  }
}
A MenuItem is then solely responsible for serving as the thing that opens a dialog. The dialog, in turn, is then solely responsible for being a dialog:
class OpenDialog {
  String name = "Open";

  var parent, ice, store;
  OpenDialog(Full full) { /* ... */ }

  open() { /* ... */ }
  _openProject(title) { /* ... */ }
}
To put this all together, I have to inject the dialog into the menu item:
new MenuItem(new OpenDialog(this)).el;
This is a little more verbose than the original, combined menu item and dialog:
new OpenDialog(this).el;
Then again, this is the benefit of helper methods:
get _openDialog=> new MenuItem(new OpenDialog(this)).el;
This does have the advantage of being explicit about the menu item's strategy. In this case, it is perfectly clear that the strategy for this particular menu item is to create an open project dialog. I really like that approach.

That said, Dart affords me a few other options. I could add a factory constructor to the OpenDialog class:
class OpenDialog {
  String name = "Open";

  var parent, ice, store;
  OpenDialog(Full full) { /* ... */ }
  factory OpenDialog.menu_item(full) {
    var dialog = new OpenDialog(full);
    return new MenuItem(dialog);
  }

  open() { /* ... */ }
  _openProject(title) { /* ... */ }
}
This would make constructing objects a little more compact:
get _openDialog=> new OpenDialog.menu_item(this).el;
Now that I think about it, I could even return the DOM Element from a factory:
class OpenDialog {
  String name = "Open";

  var parent, ice, store;
  OpenDialog(Full full) { /* ... */ }
  factory OpenDialog.menu_element(full) {
    var dialog = new OpenDialog(full),
        menu_item = new MenuItem(dialog);
    return menu_item.el;
  }

  open() { /* ... */ }
  _openProject(title) { /* ... */ }
}
Then constructing a menu with a specific dialog strategy becomes:
get _openDialog=> new OpenDialog.menu_element(this);
I really like that. It is nice and compact. It conveys the meaning well. It also keeps the menu item and dialog responsibilities separate.

That said, there are drawbacks. If I opt for a Dialog template that really is just a dialog template, then I would need to define this factory constructor in each concrete class. I will sleep on the ultimate decision, but it is nice to have so many palatable options available to me.


Day #764

Sunday, May 26, 2013

Bikeshedding in Dart

‹prev | My Chain | next›

I am still not sold on my one-class-per-dialog approach to the full-screen ICE Code Editor. I am developing in a branch, so no harm done if I abandon the approach. Still, I feel like some change has to be made as, even at this relatively early stage, the Full class is getting unwieldy.

With the new one-class-per-dialog approach, I am able to build the menu like:
class Full {
  // ...
  _showMainMenu() {
    var menu = new Element.html('<ul class=ice-menu>');
    el.children.add(menu);

    menu.children
      ..add(new OpenDialog(this).el)
      ..add(new NewProjectDialog(this).el)
      ..add(new RenameDialog(this).el)
      ..add(new CopyDialog(this).el)
      ..add(new SaveMenu(this).el)
      ..add(new ShareDialog(this).el)
      ..add(new RemoveDialog(this).el)
      ..add(new DownloadDialog(this).el)
      ..add(new HelpDialog(this).el);
  }
}
That is an improvement over the previous approach, primarily because the dialog code (the el, the dialog HTML, and the associated dialog actions) was all in this same Full class as a long series of private methods. Still...

This is probably bikeshedding, but the new, this and el stand out thanks to their repetitiveness. I could try a Dart factory constructor for each (e.g. new OpenDialog.el(this), but that would not help with the new or the this and probably would not help all that much with the .el.

I think about library global methods, but this would only leave me with something along the lines of:

    menu.children
      ..add(openDialogElement(this))
      ..add(newProjectDialogElement(this))
      ..add(renameDialogElement(this))
      ..add(copyDialogElement(this))
      ..add(saveMenuElement(this))
      ..add(shareDialogElement(this))
      ..add(removeDialogElement(this))
      ..add(downloadDialogElement(this))
      ..add(helpDialogElement(this));
There is still repetition of (this) and, to a lesser extent, Element.

So I switch to private methods in Full:
class Full {
  // ...
  _showMainMenu() {
    var menu = new Element.html('<ul class=ice-menu>');
    el.children.add(menu);

    menu.children
      ..add(_openDialog)
      ..add(_newProjectDialog)
      ..add(_renameDialog)
      ..add(_copyDialog)
      ..add(_saveMenu)
      ..add(_shareDialog)
      ..add(_removeDialog)
      ..add(_downloadDialog)
      ..add(_helpDialog);
  }

  get _openDialog=> new OpenDialog(this).el;
  get _newProjectDialog=> new NewProjectDialog(this).el;
  get _renameDialog=> new RenameDialog(this).el;
  get _copyDialog=> new CopyDialog(this).el;
  get _saveMenu=> new SaveMenu(this).el;
  get _shareDialog=> new ShareDialog(this).el;
  get _removeDialog=> new RemoveDialog(this).el;
  get _downloadDialog=> new DownloadDialog(this).el;
  get _helpDialog=> new HelpDialog(this).el;
}
That I like. I move the repetition out of the main flow of the code, leaving the main menu code nice and readable. In the end, I extracted each of those dialog classes out from a series of private methods only to bring them back. Still, better a one-line private method than dozens of lines for each dialog.

Sometimes bikeshedding pays off. If only there was a way to tell when bikeshedding was likely to be worthwhile...

Day #763

Saturday, May 25, 2013

Limiting the Impact of Class Explosion in Dart

‹prev | My Chain | next›

I am considering refactoring the Dart code in the ICE Code Editor. So far, so good with extracting dialogs and sub-menus out of private methods and into classes. I started with the extracted class in the same file as the original class. The full.dart file contains:
part of ice;

class Full { /* ... */ }
class ProjectsDialog { /* ... */ }
_hideMenu() { /* ... */ }
_hideDialog() { /* ... */ }
As the “part of” directive at the top indicates, this is part of the library's main file, ice.dart:
library ice;
// library import statement...
part 'editor.dart';
part 'store.dart';
part 'gzip.dart';
part 'full.dart';
I am inclined to make the ProjectsDialog class part of the full.dart code:
part of ice;

part 'full/projects_dialog.dart';

class Full { /* ... */ }
That will not work, though. Dart files can only be part of libraries, not other parts. Even if they could, I would still effectively be importing the ProjectsDialog class in to the main library.

I suspect that I may want to split the ICE package into not one, but two libraries. The first would be for the full-screen version of ICE and would be imported into other code as import 'package:ice_code_editor/full.dart'. The second would be the embedded version and would be imported as import 'package:ice_code_editor/embedded.dart'. Both libraries could then share things like the underlying Editor, Gzip and Store classes.

Still, I am unsure if that is necessary and I have not even started on the embedded version of the code, so I will wait to do that another day. For now, I still need to figure out how to organize things and how to keep the ProjectsDialog hidden from the outside.

I move the ProjectsDialog into a new file: full/projects_dialog.dart, which is part of the main ICE library:
part of ice;
class ProjectsDialog { /* ... */ }
Then, in ice.dart, I complete the part/part-of pair:
library ice;
// other imports here...
part 'editor.dart';
part 'store.dart';
part 'gzip.dart';

part 'full.dart';
part 'full/projects_dialog.dart';
And, since full.dart and full/projects_dialog.dart are both part of the same library, the Full class can still use ProjectsDialog as before.

That seems fine, but how can I hide ProjectsDialog from the outside world? I suppose that I do not need to prevent access from the outside world—it does not need to be private. Maybe it is sufficient to keep it out of the public documentation:



That can be fixed with an internal tool, tool/dartdoc, that generates specialized documentation:
#!/usr/bin/env sh

dartdoc \
  --package-root=packages \
  --exclude-lib=js,js.wrapping,meta,metadata \
  lib/ice.dart

mv docs/ice.html docs/dev.html

cat docs/dev.html | \
    grep -v Dialog.html | \
    grep -v ice/Ace \
  > docs/ice.html
With that, I have a simple solution for de-publicizing my internal classes:



That seems a reasonable approach. I can now extract as many menu and dialog classes as I like without polluting the documentation.

Day #762

Friday, May 24, 2013

Rethinking DOM Menus in Dart

‹prev | My Chain | next›

My house is a mess and I am not quite sure how to fix it. The house in this case is one of the classes in the Dart version of the ICE Code Editor. This is not a Dart problem—I had the same situation in the JavaScript version. But the problem is more obvious in Dart.

Much of the effort of late in ICE has gone into the full-screen, lite-IDE that is used in 3D Game Programming for Kids. The class, Full starts off well enough:
class Full {
  Full() { /* ... */ }
  Future get editorReady => _ice.editorReady;
  String get content => _ice.content;
  void set content(data) => _ice.content = data;
  // ...
}
(the Full() method is the constructor)

As we have been building out the various menus and dialogs that go into an IDE, things have gone a little wrong. The menus and dialogs are clearly private methods—no external class should ever need to open the full-screen share dialog. Since Dart has first-class support for private methods, I have been dutifully marking these methods as private. It turns out that there are a lot of them:
class Full {
  // ...
  _attachToolbar() { /* ... */ }
  _attachMainMenuButton(parent) { /* ... */ }
  _attachKeyboardHandlers() { /* ... */ }
  _attachMouseHandlers() { /* ... */ }
  _showMainMenu() { /* ... */ }
  _hideMenu() { /* ... */ }
  _hideDialog() { /* ... */ }

  get _newProjectMenuItem { /* ... */ }
  _openNewProjectDialog() { /* ... */ }
  _saveNewProject() { /* ... */ }

  get _projectsMenuItem { /* ... */ }
  _openProjectsMenu() { /* ... */ }
  _openProject(title) { /* ... */ }

  get _saveMenuItem { /* ... */ }
  void _save() { /* ... */ }

  get _shareMenuItem { /* ... */ }
  _openShareDialog() { /* ... */ }
}
I am not quite certain how best to reduce the noise. The most obvious thing—the thing that I have been threatening since this was in JavaScript—is to move dialogs and their associated actions into self-contained classes. The biggest unknown for me is how to allow communication back into the main class.

The dialog that needs the most access is the Projects dialog, whose current entry point is _projectsMenutItem(). In addition to doing menu-like things, it needs to read from and write to the localStorage Store class. It also needs to be able to update the code editor when switching between projects. Since that seems the hardest, I start with it.

The entry point is still the menu list item that goes on the main menu. This will have to be exposed as a getter, which I call el:
class ProjectsDialog {
  // ...
  Element get el {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> _openProjectsMenu());
  }
  // ...
}
I pull in the _openProjectsMenu() method without change, along with the _openProject() method. Already the cohesion of these methods is improved. To continue to work, they need access to the parent element of the Full editor, as well as the editor itself and the data store. So I define a constructor that accepts all three:
class ProjectsDialog {
  var parent, ice, store;
  ProjectsDialog(this.parent, this.ice, this.store);
  Element get el {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> _openProjectsMenu());
  }
  // ...
}
With that, I can return the main menu, which is still in the Full class, and inject the new ProjectsDialog as:
class Full {
  // ...
  _showMainMenu() {
    var menu = new Element.html('<ul class=ice-menu>');
    el.children.add(menu);

    menu.children
      ..add(new ProjectsDialog(el, _ice, _store).el)
      ..add(_newProjectMenuItem)
      ..add(new Element.html('<li>Rename</li>'))
      ..add(new Element.html('<li>Make a Copy</li>'))
      ..add(_saveMenuItem)
      ..add(_shareMenuItem)
      ..add(new Element.html('<li>Download</li>'))
      ..add(new Element.html('<li>Help</li>'));
  }
  // ...
}
And that works. All of my tests still pass. I even give it a try in the sample app and it still works.

It seems a bit ugly to pass in those three parameters to the ProjectsDialog constructor. If only there was something that encapsulated those three objects… like the Full object that is creating them maybe?

Then the menu list could start as:
    menu.children
      ..add(new ProjectsDialog(this).el)
      ..add(_newProjectMenuItem)
      ..add(new Element.html('<li>Rename</li>'))
      // ...
That works, but the _ice and _store properties can no longer be private. I am not sure that is worth the change, so I call it a night here to give the idea a chance to percolate.

Regardless, I like the overall approach to extracting the menu dialogs out of the Full IDE class. This approach definitely has promise. And having a strong test suite in place to guard against regressions is invaluable.

Day #761

Thursday, May 23, 2013

Minor Refactoring under Test in Dart

‹prev | My Chain | next›

Today, I would like to start thinking about how to consolidate some of my Dart code in the ICE Code Editor. I think that it is a bit early to start breaking the sub-menus and dialogs out into Dart classes. So instead, I am going to scavenge the codebase for things that are asking to be removed or consolidated.

One of the first things that asks to come out are styles. I love using method cascades for CSS styles in Dart:
  _openNewProjectDialog() {
    var dialog = new Element.html(
        '''
        <div class=ice-dialog>
        <label>Name:<input type="text" size="30"></label>
        <button>Save</button>
        </div>
        '''
      );

    dialog.style
      ..position = 'absolute'
      ..margin = '2px'
      ..right = '20px'
      ..top = '45px'
      ..zIndex = '999';
    // ...
  }
As much as I like a good method cascade, I am setting the same style for multiple dialogs. In fact, I am setting the same style for sub-menus as well. So, reluctantly, I pull that out into vanilla CSS:
.ice-menu, .ice-dialog {
  /* ... */
  position: absolute;
  margin: 2px;
  right: 20px;
  top: 45px;
  z-index: 999;
}
Sometimes there is no substitution for a low-tech solution.

I clean out a few other minor things and then come across:
  _hideMenu() {
    if (query('.ice-menu') == null) return;
    query('.ice-menu').remove();
    // queryAll('.ice-menu').forEach((e)=> e.remove());
  }
I really dislike that conditional. It seems like that queryAll() ought to be equivalent, if not a better solution. Instead of finding one .ice-menu element, it will find them all and remove each. If there are no matching elements, which is what the current conditional is guarding, then the forEach() should be a no-op.

But I commented that out for a reason. Specifically, if I replace the current code with the queryAll() version, I get test failures:
FAIL: project menu lists project names
  Expected: List of elements contains match 'My New Project'
       but: Element list content was <[☰X, ☰, X, , , , , , , , , , , , , X, , , ]>.
Interestingly, I think that I was relying on coincidence to make the “list project names” feature work. In particular, the way that I opened the project list sub-menu was opening the menu first, then closing the main menu:
  _projectsMenuItem() {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _openProjectsMenu())
      ..onClick.listen((e)=> _hideMenu());
  }
This, and a bunch of other tests, pass if I hide all menus before opening the project menu:
  _projectsMenuItem() {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> _openProjectsMenu());
  }
In the end, the previous conditional-then-remove-the-first-element-in-the-DOM approach was less than robust. So this refactoring / cleanup has helped improve the strength of the codebase. Hopefully it has cleaned enough cruft so that I have a better handle on how to better approach those sub-menus and dialogs. I will get started on that tomorrow.


Day #760

Wednesday, May 22, 2013

Driving Dart Integration Tests

‹prev | My Chain | next›

The crazy thing about driving things with tests is that stuff gets done. I have been having a good old time playing around with the test suite in the Dart version of the ICE Code Editor. I spent so much time fiddling with the tests that I barely noticed the progress with functionality. Mostly thanks to #pairwithme sessions, the functionality of the ICE Code Editor is really coming along.

Tonight, I take a step back to see how it is working with the new test helpers that I have written over the past two nights when driving new functionality. Thanks to last night's #pairwithme session with Santiago Arias, ICE now lists the currently saved projects in the Projects menu. Tonight I would like to be able to click on old ones to make them active.

I start with a long UI workflow test:
    test("click names to switch between projects", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');

      query('input').value = 'Project #1';
      helpers.click('button', text: 'Save');

      editor.content = 'Code #1';
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Save');

      helpers.click('button', text: '☰');
      helpers.click('li', text: 'New');

      query('input').value = 'Project #2';
      helpers.click('button', text: 'Save');

      editor.content = 'Code #2';
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Save');

      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Projects');
      helpers.click('li', text: 'Project #1');

      expect(
        editor.content,
        equals('Code #1')
      );
    });
This fully exercises the full screen ICE menus by clicking the menu button (the UTF-8 ☰ symbol), then creating a new project, setting the content and saving the project. I do that twice, at which point the content for the second project is still active. I then open the Projects menu and select the first project from the list. The expectation is that the editor content will have changed to “Code #1” the content of the first project.

It is so cool that, thanks to Dart's testing facilities and some simple helpers, the test is so expressive and yet completely functional. The first time I run that test, I get a failure. The exact failure that I had hoped to get to drive this feature:
FAIL: project menu click names to switch between projects
  Expected: 'Code #1'
       but: was 'Code #2'.
How awesome is that?

Then again, maybe my UI testing expectations are just too low. Perhaps this is really how it ought to be. Regardless, this is reality in the world of Dart and I embrace it by writing the code that implements this. With that bit of smugness fully internalized, I run into all sorts of problems.

The implementation is not too hard:
    _store.forEach((title, data) {
      var project = new Element.html('<li>${title}</li>')
        ..onClick.listen((e)=> _openProject(title))
        ..onClick.listen((e)=> _hideMenu());

      menu.query('ul').children.add(project);
    });
For each item in the data store, I create a menu item for the Projects menu. Each project menu item gets a couple of handlers to open the project and hide the menu. The underlying Store class could support opening a project better, but that is not really the problem.

The problem is that, after saving items by clicking the “Save” option from the main menu, I was not closing the main menu. It would stay up causing subsequent tests to fail because the wrong menus were now active.

I could have written this test such that the localStorage store was initialized with projects in place. This would have avoided the save-not-closing-the-menu issue (though I would not have found the bug otherwise) and the subsequent implementation would likely end up exactly the same. But there is something exciting about a test that exercises the full stack in milliseconds. If I had other tests that covered similar ground, I might manually initialize the localStorage, but I really prefer to keep this around.

It turns out to be quite the effort to keep it around. The test continues to fail and is compounded with a problem running these tests solo due to js-interop concerns. Thankfully, tonight's #pairwithme pair, Daniel Gempesaw, helps me through it.

We drive the problem away with another test:
    test("clicking save closes the main menu", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text:  'Save');

      expect(queryAll('li').map((e)=> e.text).toList(), isEmpty);
    });
All that is needed to make that pass is a second on-click listener for the “Save” menu item:
  Element get _saveMenuItem {
    return new Element.html('<li>Save</li>')
      ..onClick.listen((e)=> _save())
      ..onClick.listen((e)=> _hideMenu());
  }
With that, not one, but two tests are passing. More importantly, it is now possible to switch between projects in the Dart version of the ICE Code Editor. Yay!


Day #759

Tuesday, May 21, 2013

Custom Test Matchers in Dart

‹prev | My Chain | next›

After last night, I have helpered my way to a nice looking Dart test:
    test("clicking the project menu item opens the project dialog", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Projects');

      expect(
        queryAll('div').map((e)=> e.text).toList(),
        contains(matches('Saved Projects'))
      );
    });
That's downright pretty, except… that expect() is, let's face it, plain yucky.

The thing that I am testing is kinda OK. It is a map of the text contents of <div> elements. I could do without the map(), but it's not horrible. The toList() is bothersome. It can be omitted, but it is useful to have around. For instance, if I cause an intentional failure by naming the menu "Saved Code" instead of "Saved Projects", I get a nice error message that gives me an idea of what went wrong:
FAIL: project menu clicking the project menu item opens the project dialog
  Expected: contains match 'Saved Projects'
       but: was <[☰X
          Saved Code
          , ☰, X, , , , , , , , , , , , , X, , , , 
          Saved Code
          ]>.
If I omit the toList(), then the object that I am testing is an Iterable. It still passes or fails as desired, but the failure message is less nice:
FAIL: project menu clicking the project menu item opens the project dialog
  Expected: contains match 'Saved Projects'
       but: was <Instance of 'MappedListIterable':554001401>.
So I leave toList() as a bit of test code ugliness so that I get nicer test code output. I can live with that.

The expected value is not quite as nice. Dart provides a nice matcher for lists: contains(). So in this case I am expecting a list that contains something. That something is anything that matches the string 'Saved Projects'. It works, but it is hard to read.

But hard to read matchers are what custom matchers are for. For this expectation, I may not even need to write an entirely new matcher. Instead, I start by inheriting from CustomMatcher. These nifty little things set a description ("List of elements") and a feature name ("Element list content") in the constructor:
class ElementListMatcher extends CustomMatcher {
  ElementListMatcher(matcher) :
      super("List of elements", "Element list content", matcher);

  featureValueOf(elements) => elements.map((e)=> e.text).toList();
}
I then pick a value to extract from the actual value. If the actual value is a list of elements, then the above extracts the text contents in List form. That is just what I had to do by hand in my test, but all of the ugliness of mapping and converting from an iterable to a list is done in the custom matcher.

I then create a top-level helper that uses this matcher to check the extracted/featured list to see if it contains a match:
elements_contain(Pattern content) =>
  new ElementListMatcher(contains(matches(content)));
Those are two pretty simple helpers that let me rewrite my test entirely with helpers as:
    test("clicking the project menu item opens the project dialog", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Projects');

      expect(
        queryAll('div'),
        helpers.elements_contain('Saved Projects')
      );
    });
That is pretty all the way through: I click a button with the menu icon, I click the "Projects" menu item, then I expect one of the <div> tags to contain the text "Saved Projects". Wonderful!

And best of all it works!

If I again intentionally make my test fail, I get:
FAIL: project menu clicking the project menu item opens the project dialog
  Expected: List of elements contains match 'Saved Projects'
       but: Element list content was <[☰X
          Saved Code
          , ☰, X, , , , , , , , , , , , , X, , , , 
          Saved Code
          ]>.
That is even more expressive than what I had when I manually extracted a list to test and the test content is easier to read. The use of the helpers prefix from last night is the only remaining noise (and I still think it worth keeping about). All in all, these test matchers are pretty powerful.


Day #758

Monday, May 20, 2013

Dart Test Helpers

‹prev | My Chain | next›

I find myself repeating a lot of the same test actions in my ICE Code Editor test suite. This seems a fine opportunity to explore test helpers in Dart unit tests.

The one in particular that I find myself doing a lot is clicking on element, usually with particular content:
      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();
My first instinct is that the helpers should be a separate library that is imported into my test suite. The main reason is that I can import with an “as” prefix to make it patently obvious where the helper methods live. So, in my main ice_test.dart main test file, I add the import statement:
library ice_test;

import 'package:unittest/unittest.dart';
// ...
import 'helpers.dart' as helpers;
import 'package:ice_code_editor/ice.dart';

main(){
  // tests go here...
}
In helpers.dart, I start with a single click() function:
library ice_test_helpers;

import 'dart:html';

void click(String selector, {text}) {
  if (text == null) return query(selector).click();

  queryAll(selector).
    firstWhere((e)=> e.text==text).
    click();
}
The click function requires a string selector that will be used to query for elements to click. If no text is specified—if the optional, named parameter text is null—then I query for the first matching selector and click it. If the text parameter is specified, then I query for all matching selectors, find the first that contains the supplied text and click that.

I continue to use the firstWhere() because it will throw an exception if no matching element is found. I may want to bundle that into a new exception that makes it more obvious what has gone wrong in the test, but I leave it for now.

With that, I can change the test that verifies one of the ways to close a menu:
    test("the menu button closes the projects dialog", (){
      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();

      queryAll('li').
        firstWhere((e)=> e.text=='Projects').
        click();

      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();

      expect(
        queryAll('div').map((e)=> e.text).toList(),
        isNot(contains(matches('Saved Projects')))
      );
    });
Instead, I can write that as:
    test("the menu button closes the projects dialog", (){
      helpers.click('button', text: '☰');
      helpers.click('li', text: 'Projects');
      helpers.click('button', text: '☰');

      expect(
        queryAll('div').map((e)=> e.text).toList(),
        isNot(contains(matches('Saved Projects')))
      );
    });
Holy clearer intent Batman! It is much easier to see that this test clicks the menu button, then the Projects menu item, then the menu button again.

I might have omitted the “helpers” prefix from my import statement and thus been able to treat click() as a top-level function. I tend to think that the prefix will aid in long-term maintainability of the test suite as there will never be a question as to the source of the helper function.


Day #757

Sunday, May 19, 2013

Verifying Persistent Browser Storage with Dart Integration Tests

‹prev | My Chain | next›

One of the crazy things about blogging every day—even the routine stuff of BDDing a new feature—is my ability to run into seemingly daily problems. Last night, I realized that I could not simulate keyboard events in Dart tests. Thanks to Damon Douglas, my #pairwithme partner, I have a solution. It is not an ideal solution, but it will suffice.

So tonight, I try to find yet another daily problem.

I am going to attempt to drive the saving of projects in the ICE Code Editor with tests. In some ways, this is a useless feature because the editor will (eventually) auto-save on every change. Still, it makes people feel more comfortable if it is around. Also, it is a good opportunity for mayhem as this is the first time that I need to use the Store class, which interfaces with localStorage. Problems are sure to abound!

So I start with a test. I create a new full-screen editor instance, set the content, save it with the menu and then start a new instance. The new instance should retain the contents of the previous session by virtue of the Store class that we wrote a couple of weeks ago:
  group("saving projects", (){
    var editor;

    setUp(()=> editor = new Full(enable_javascript_mode: false));
    tearDown(()=> document.query('#ice').remove());

    test("a saved project is loaded when the editor starts", (){
      editor.content = 'asdf';

      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();

      queryAll('li').
        firstWhere((e)=> e.text=='Save').
        click();

      document.query('#ice').remove();
      editor = new Full(enable_javascript_mode: false);

      expect(editor.content, equals('asdf'));
    });
  });
I probably need to write some helper functions for clicking buttons and menu items, but I will leave that for tomorrow. I have other problems tonight. Specifically, since there is no save action yet, I get a nice failing test:
FAIL: saving projects a saved project is loaded when the editor starts
  Expected: 'asdf'
       but: was ''.
In the Full class for full-screen editing, I need to initialize an instance of the Store class. The constructor is just the place for this:
class Full {
  Editor _ice;
  Store _store;

  Full({enable_javascript_mode: true}) {
    // ...
    _ice = new Editor('#ice', enable_javascript_mode: enable_javascript_mode);
    _store = new Store();
    // ...
  }
  // ...
}
Next, I need a menu item that will do the saving of the contents:
  Element get _saveMenuItem {
    return new Element.html('<li>Save</li>')
      ..onClick.listen((e)=> _save());
  }

  void _save() {
    _store['asdf'] = {'code': content};
  }
The _saveMenuItem getter returns an <li> element that, when clicked, calls the _save() method, which is responsible for updating the actual store. The name is obviously wrong and something that a subsequent test will have to drive. But it should suffice for now.

With that, the only other thing that I need to do is update the constructor to set the content from the store (if present):
  Full({enable_javascript_mode: true}) {
    // ..
    _ice = new Editor('#ice', enable_javascript_mode: enable_javascript_mode);
    _store = new Store();
    // ...
    editorReady.then((_)=> content = _store.projects.first['code']);
  }
This makes use of the underlying Future that completes when the JavaScript editor (ACE) finishes loading and doing its own initialization. Of course, that takes some time, which causes my test to still fail. In the test, I also have to wait for the future to complete before checking that the content is retained. Dart may not do me any favors when testing keyboard events, but it does make testing asynchronous events a breeze:
  group("saving projects", (){
    var editor;

    setUp(()=> editor = new Full(enable_javascript_mode: false));
    tearDown(() {
      document.query('#ice').remove();
      new Store().clear();
    });

    test("a saved project is loaded when the editor starts", (){
      editor.content = 'asdf';

      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();

      queryAll('li').
        firstWhere((e)=> e.text=='Save').
        click();

      document.query('#ice').remove();
      editor = new Full(enable_javascript_mode: false);

      _test(_) {
        expect(editor.content, equals('asdf'));
      };
      editor.editorReady.then(expectAsync1(_test));
    });
The expectAsync1() test function declares to my test that there will be an asynchronous call (and that it will receive one argument) and that the test should not consider itself done until that wrapper is called. Once the expectAsync1() function is called by the editorReady completer, then the private _test() method is called, which checks the editor content.

And it works! If I run the test, I have now added one more passing test to the test suite:
PASS: saving projects a saved project is loaded when the editor starts

All 31 tests passed. 
Best of all, if I remove the restore-from-storage future in the constructor, this test fails, which gives me more confidence in the value of this test.

There is definitely more work ahead of me, but it is pretty exciting to have taken the next step toward a working persistent store in the full-screen version of ICE. It is even more exciting to have it working under a strong test to help guard against regressions.


Day #756

Saturday, May 18, 2013

BDD a Dart Menu

‹prev | My Chain | next›

The sharing feature in the ICE Code Editor is not quite done, but I am going to risk starting a new feature tonight. It is a bit of a concern having multiple features under development at the same time, but it could be a good thing for my #pairwithme sessions.

The most interesting next feature is the project menu. What makes it interesting is that it combines three different classes in the project: the core editor, the localStorage, and the full-screen IDE. I start with four empty tests that still start me on the way:
  group("project menu", (){
    skip_test("clicking the project menu item opens the project dialog", (){});
    skip_test("the escape key closes the project dialog", (){});
    skip_test("the menu button closes the projects dialog", (){});
    skip_test("contains a default project on first load", (){});
  });
I will probably leave that last one, which begins to exercise the localStorage component, until tonight's #pairwithme session, but hopefully I can get through the rest.

I start by adding the usual setup and teardown:
  group("project menu", (){
    setUp(()=> new Full(enable_javascript_mode: false));
    tearDown(()=> document.query('#ice').remove());

    skip_test("clicking the project menu item opens the project dialog", (){});
    // ...
  });
The enable_javascript_mode option is a test-only feature. It disables JavaScript syntax highlighting because the underlying ACE code editor uses web workers, which do not work for file:// URLs. Since the tests are run from a URL like file:///home/chris/repos/ice-code-editor/test/index.html, the web workers are guaranteed to fail, adding messy warnings to the test output.

I convert the first test from a skip_test() to the real thing and set my expectations:
    test("clicking the project menu item opens the project dialog", (){
      queryAll('button').
        firstWhere((e)=> e.text=='☰').
        click();

      queryAll('li').
        firstWhere((e)=> e.text=='Projects').
        click();

      expect(
        queryAll('div').map((e)=> e.text).toList(),
        contains(matches('Saved Projects'))
      );
    });
This test says that, after clicking the menu button and the “Projects” menu item, I expect that the project sub-menu will be open. I like using the firstWhere() method when looking for elements to click in tests—it throws exceptions if there is no matching element. That is what happens in this case:
FAIL: project menu clicking the project menu item opens the project dialog
  Caught Bad state: No matching element
  Object&ListMixin.firstWhere                                    dart:collection 561:5
  full_tests.<anonymous closure>.<anonymous closure>             file:///home/chris/repos/ice-code-editor/test/full_test.dart 90:19
This is because I have named the menu item “Open.” I rename it to “Projects” and now I have a failure that the projects dialog is not open:
FAIL: project menu clicking the project menu item opens the project dialog
  Expected: contains match 'Saved Projects'
       but: was <[☰XNewProjectsSaveMake a CopyShareDownloadHelp, ☰, X, , , , , , , , , , , , , X, , , ]>.
  
  full_tests.<anonymous closure>.<anonymous closure>             file:///home/chris/repos/ice-code-editor/test/full_test.dart 93:13
I make that pass by calling a new private method to add the Projects menu item:
    menu.children
      ..add(new Element.html('<li>New</li>'))
      ..add(_projectsMenuItem())
      // ...
The new private method creates the menu item and attaches a click listener:
  _projectsMenuItem() {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e) => _openProjectsMenu());
  }
Finally, the menu that opens the project menu and makes the test pass:
  _openProjectsMenu() {
    var menu = new Element.html(
        '''
        <div class=ice-menu>
        <h1>Saved Projects
        </div>
        '''
    );

    el.children.add(menu);

    menu.style
      ..maxHeight = '560px'
      ..overflowY = 'auto'
      ..position = 'absolute'
      ..right = '17px'
      ..top = '60px'
      ..zIndex = '1000';
  }
Both the CSS and the code seem ripe to extraction. I have the feeling that this is not the last menu that I need. Still, I will worry about generalizing the behavior later.

Update: The close-menu-with-escape proves to be extremely difficult. But, thanks to Damon Douglas, tonight's #pairwithme partner, I have a solution.

As best I can tell, there is no way to properly simulate keyboard events in Dart. It is possible to create an instance of KeyboardEvent, but it is not possible to set the charCode of that event—either in the constructor or via a setter. Craziness!

We found that it is possible to specify a key identifier in the constructor. If the character code for the escape key is 27, then I can create an escape keyboard event with:
    test("the escape key closes the project dialog", (){
      // open the project menu

      document.body.dispatchEvent(
        new KeyboardEvent(
          'keyup',
          keyIdentifier: new String.fromCharCode(27)
        )
      );

      expect(
        queryAll('div').map((e)=> e.text).toList(),
        isNot(contains(matches('Saved Projects')))
      );
    });
Unfortunately, this still does not set a charCode on the resulting event. Nevertheless, it does set information that can be passed from the test into the application code. To get this test to pass, we have to check both the charCode and the keyIdentifier values in the application code:
    document.onKeyUp.listen((e) {
      if (e.keyCode == 27) _hideMenu();
      if (e.$dom_keyIdentifier.codeUnits.first == 27) _hideMenu();
    });
It is never a good thing to call one of the dollar-sign methods in Dart. It is also not a good thing to write code just to make a test pass. There is no way that second conditional will get triggered in live code and it does contain the same spirit as the real event conditional on the previous line.

Still, I will be much happier once there is a real way to test keyboard events in Dart. This is a useful first approximation, but nothing beats the real thing.


Day #755

Friday, May 17, 2013

Headless Testing in Dart (Take Two)

‹prev | My Chain | next›

I am pretty jazzed about the progress in both the ICE Code Editor and the test suite that is driving much of its development. I really have the sense that I have the beginnings of a robust codebase. I love Dart. One of the things that I do not have is the ability to test headlessly. This is something that worked way back in the day, but stopped at some point due to a bug in unittest.

Actually, thanks to some feedback on that bug report, it may just be a question of some additional test setup. To test that out, I first upgrade my Dart SDK and unittest. I've been stuck on:
ice-code-editor git:(master) dart --version
Dart VM version: 0.5.0.1_r21823 (Mon Apr 22 14:02:11 2013)
This keeps me back on the 0.5.0 version of unittest:
➜  ice-code-editor git:(master) ls -l packages 
total 16
lrwxrwxrwx 1 chris chris 66 May 16 00:21 browser -> /home/chris/.pub-cache/hosted/pub.dartlang.org/browser-0.5.0+1/lib
lrwxrwxrwx 1 chris chris  6 May 16 00:21 ice_code_editor -> ../lib
lrwxrwxrwx 1 chris chris 60 May 16 00:21 js -> /home/chris/.pub-cache/hosted/pub.dartlang.org/js-0.0.22/lib
lrwxrwxrwx 1 chris chris 63 May 16 00:21 meta -> /home/chris/.pub-cache/hosted/pub.dartlang.org/meta-0.5.0+1/lib
lrwxrwxrwx 1 chris chris 67 May 16 00:21 unittest -> /home/chris/.pub-cache/hosted/pub.dartlang.org/unittest-0.5.0+1/lib
After downloading the latest version of Dart, I have SDK version 0.5.7. A pub install then installs the 0.5.7 version of unittest (among others):
➜  ice-code-editor git:(master) dart --version
Dart VM version: 0.5.7.3_r22659 (Mon May 13 20:57:19 2013) on "linux_x64"
➜  ice-code-editor git:(master) pub update
Resolving dependencies...
Downloading unittest 0.5.7...
Downloading browser 0.5.7...
Downloading meta 0.5.7...
Dependencies updated!
I did not realize that it was possible to pin pub packages like this. I doubt that I will ever need to do something like that, but a quick inspection of unittest's pubspec.yaml shows that the environment property does the trick:
➜  ice-code-editor git:(master) cat ~/.pub-cache/hosted/pub.dartlang.org/unittest-0.5.7/pubspec.yaml 
name: unittest
author: "Dart Team <misc@dartlang.org>"
homepage: http://www.dartlang.org
documentation: http://api.dartlang.org/docs/pkg/unittest
description: >
 A library for writing dart unit tests.
dependencies:
  meta: any


version: 0.5.7
environment:
  sdk: ">=0.5.7"
Anyhow, now that I am on latest, I can see if things have changed with headless Dart testing. Headless testing is done with the DumpRenderTree tool that is bundled with Dart:
➜  ice-code-editor git:(master) DumpRenderTree test/index.html 
CONSOLE MESSAGE: unittest-suite-wait-for-done
Content-Type: text/plain
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x571
      RenderBlock {H1} at (0,0) size 784x37
        RenderText {#text} at (0,0) size 69x36
          text run at (0,0) width 69: "Test!"
#EOF
#EOF
Since there is no test output, it seems that the bug still exists. So I add the suggested code to the web page that provides the DOM context in which the tests run:
<html>
<head>
  <title>ICE Test Suite</title>
  <script type='text/javascript'>
    var testRunner = window.testRunner || window.layoutTestController;
    if (testRunner) {
      function handleMessage(m) {
        if (m.data == 'done') {
          testRunner.notifyDone();
        }
      }
      testRunner.waitUntilDone();
      window.addEventListener("message", handleMessage, false);
    }
    if (navigator.webkitStartDart) {
      navigator.webkitStartDart();
    }
  </script>
  <script type="application/dart" src="ice_test.dart"></script>
</head>
<body>
<h1>Test!</h1>
</body>
</html>
But, when I run this in DumpRenderTree, the output hangs with:
➜  ice-code-editor git:(master) ✗ DumpRenderTree test/index.html
CONSOLE MESSAGE: unittest-suite-wait-for-done
CONSOLE MESSAGE: line 54: Uncaught ReferenceError: ReceivePortSync is not defined
CONSOLE MESSAGE: Exception: The null object does not have a method 'callSync'.

NoSuchMethodError : method not found: 'callSync'
Receiver: null
Arguments: [GrowableObjectArray len:0]
CONSOLE MESSAGE: Exception: The null object does not have a method 'callSync'.

NoSuchMethodError : method not found: 'callSync'
Receiver: null
Arguments: [GrowableObjectArray len:0]
After some time, this eventually returns with:
FAIL: Timed out waiting for notifyDone to be called
FAIL: Timed out waiting for notifyDone to be called
I have seen the ReceivePortSync error before when testing js-interop. To fix, I cannot manually start the Dart engine with navigator.webkitStartDart()—I need a more complete start of the engine. So I remove the conditional above, replacing it with a <script> source that pulls in Dart's browser/dart.js:
  <script type='text/javascript'>
    var testRunner = window.testRunner || window.layoutTestController;
    if (testRunner) {
      function handleMessage(m) {
        if (m.data == 'done') {
          testRunner.notifyDone();
        }
      }
      testRunner.waitUntilDone();
      window.addEventListener("message", handleMessage, false);
    }
    // if (navigator.webkitStartDart) {
    //   navigator.webkitStartDart();
    // }
  </script>
  <script src="packages/browser/dart.js"></script>
  <script type="application/dart" src="ice_test.dart"></script>
With that, I do see my test output:
➜  ice-code-editor git:(master) ✗ DumpRenderTree test/index.html
CONSOLE MESSAGE: unittest-suite-wait-for-done
CONSOLE MESSAGE: PASS: defaults defaults to auto-update the preview
CONSOLE MESSAGE: PASS: defaults starts an ACE instance
CONSOLE MESSAGE: PASS: defaults defaults to disable edit-only mode
CONSOLE MESSAGE: PASS: content can set the content
...
CONSOLE MESSAGE:
CONSOLE MESSAGE: All 29 tests passed.
CONSOLE MESSAGE: unittest-suite-success
Unfortunately, it still hangs at this point. Eventually, I get notifyDone errors:
FAIL: Timed out waiting for notifyDone to be called
FAIL: Timed out waiting for notifyDone to be called
Taking a closer look at the supplied runner code, I see that, to notify the test runner that it is done, a message with the contents "done" needs to be posted:
      function handleMessage(m) {
        if (m.data == 'done') {
          testRunner.notifyDone();
        }
      }
I am unsure if the unittest library is supposed to post that message. Regardless of whether it is supposed to send the message, it is not. So it seems that I need to add a way to poll for the test cases to be complete (there are no tests-done Futures in unittest).

To poll, I import the dart:async library for access to the Timer class. The pollForDone() function can then be added to my test suite as:
library ice_test;

import 'package:unittest/unittest.dart';
import 'dart:html';
import 'dart:async';
import 'package:ice_code_editor/ice.dart';
// ...
main(){
  // Run tests here...
  pollForDone(testCases);
}

pollForDone(List tests) {
  if (tests.every((t)=> t.isComplete)) {
    window.postMessage('done', window.location.href);
    return;
  }

  var wait = new Duration(milliseconds: 100);
  new Timer(wait, ()=> pollForDone(tests));
}
Given the list of all test cases, this checks if every one of them is complete. If they are, then it posts the 'done' message for the test runner and exits. If some tests are incomplete, then it waits for 100 milliseconds before trying again.

And that does the trick! The test suite still runs in Dartium and now it runs headless as well:
➜  ice-code-editor git:(master) ✗ time DumpRenderTree test/index.html
CONSOLE MESSAGE: unittest-suite-wait-for-done
CONSOLE MESSAGE: PASS: defaults defaults to auto-update the preview
CONSOLE MESSAGE: PASS: defaults starts an ACE instance
CONSOLE MESSAGE: PASS: defaults defaults to disable edit-only mode
CONSOLE MESSAGE: PASS: content can set the content
...
CONSOLE MESSAGE: PASS: sharing menu should close when share dialog activates

CONSOLE MESSAGE:
CONSOLE MESSAGE: All 29 tests passed.
CONSOLE MESSAGE: unittest-suite-success
...
DumpRenderTree test/index.html  0.77s user 0.10s system 100% cpu 0.870 total
The pollForDone() function is a bit of a hassle, but one that I am happy to live with if it gets me headless testing. Still, I will post that solution back onto the bug to see if there is some way that unittest could post the 'done' message itself.


Day #754

Thursday, May 16, 2013

Getting Started BDDing Dart HTML Workflows

‹prev | My Chain | next›

Now that I have a beautiful Dart test suite for ICE Code Editor, I would like to try driving a little UI workflow.

For my first test, I write something that is decidedly not behavior driving. I check for the presence of a button:
  group("main toolbar", (){
    setUp(()=> new Full(enable_javascript_mode: false));
    tearDown(()=> document.query('#ice').remove());

    test("it has a menu button", (){
      var buttons = document.queryAll('button');
      expect(buttons.any((e)=> e.text=='☰'), isTrue);
    });
  });
In other words, when the full screen editor first starts up, there should be a menu button available.

I am not going to test for location or z-index or any formatting here. I will visually inspect that they exist. It might be nice if Dart supported something like a visible getter, but perhaps that is something for a library. Anyhow I will get the toolbar in place, then I will write another test to drive some UI behavior.

But first, I need to get the test passing:
FAIL: main toolbar it has a menu button 
  Expected: true
       but: was <false>.
I make that pass by modifying the Full class to attach a toolbar on construction. And, I have the toolbar create the menu button:
class Full {
  Full({enable_javascript_mode: true}) {
    // ...
    _attachToolbar();
  }

  _attachToolbar() {
    var el = new Element.html('<div class=ice-toolbar>');
    el.style
      ..position = 'absolute'
      ..top = '10px'
      ..right = '20px'
      ..zIndex = '999';

    _attachMenuButton(el);

    document.body.nodes.add(el);
  }

  _attachMenuButton(parent) {
    var el = new Element.html('<button>☰');
    parent.children.add(el);
  }
}
That gets my first test passing:
PASS: main toolbar it has a menu button
Now, I am ready to write my first UI workflow test—nothing fancy, just that clicking this button brings up the menu that includes a “Help” item:
    test("clicking the menu button brings up the menu", (){
      var menu_button = queryAll('button').
        firstWhere((e)=> e.text=='☰');

      menu_button.click();
      var menu = queryAll('li').
        firstWhere((e)=> e.text.contains('Help'));

      expect(menu, isNotNull);
    });
And make it pass with:
  toggleMenu() {
    var el = new Element.html('<ul class=ice-menu>');
    document.body.children.add(el);

    el.style
      ..position = 'absolute'
      ..right = '17px'
      ..top = '55px'
      ..zIndex = '999';

    el.children
      ..add(new Element.html('<li>New</li>'))
      ..add(new Element.html('<li>Open</li>'))
      ..add(new Element.html('<li>Save</li>'))
      ..add(new Element.html('<li>Make a Copy</li>'))
      ..add(new Element.html('<li>Share</li>'))
      ..add(new Element.html('<li>Download</li>'))
      ..add(new Element.html('<li>Help</li>'));
  }
That was easy!

My #pairwithme, Srdjan Pejic, and I spend the rest of the evening building out a few of those menu elements—BDDing as much as possible along the way. So far, I have to say that I really like driving UI changes like this. It will be interesting to see if my happiness level remains high as the UI (and complexity) grows.


Day #753

Wednesday, May 15, 2013

One Main() for Great Good of Dart Tests

‹prev | My Chain | next›

Thanks largely to Damon Douglas, my #pairwithme pair, I have the test suite for the Dart version of the ICE Code Editor in much better shape.

When we started last night all the tests passed, but there was lots of red and other weird error-like output when I ran the test suite:



When we finished, all of that was gone. Well, almost all of it:



It was only one red line, but we gave it a good try to eliminate that one red line. Unfortunately, it was late and everything we did only made things worse. Compounding the problems was that the single red line often grew into a full stack trace dripping with js-interop influence:
Exception: Non unique ID: dart-0
Stack Trace: #0      Proxy._forward (file:///home/chris/repos/ice-code-editor/test/packages/js/js.dart:1043:22)
#1      Proxy.noSuchMethod (file:///home/chris/repos/ice-code-editor/test/packages/js/js.dart:1033:20)
#2      Ace.edit (file:///home/chris/repos/ice-code-editor/test/packages/ice_code_editor/editor.dart:238:65)
#3      Editor._startJsAce (file:///home/chris/repos/ice-code-editor/test/packages/ice_code_editor/editor.dart:180:20)
#4      Editor._startAce.<anonymous closure> (file:///home/chris/repos/ice-code-editor/test/packages/ice_code_editor/editor.dart:171:52)
Last night, we tried to tweak the code to prevent the problem. Fresh eyes today make me realize that I am probably going to have to re-organize my test suite to eliminate this problem.

What leads me to believe that a code re-organization is in the cards is that the various specs—for the core editor, the storage, the full-screen editor—all run fine in isolation. This particular problem only pops up when two different test groups try to run the same js-interop code. I do not think this is a limitation of js-interop or of Dart. I think it is more a case that the organization scheme that I have chosen was, in hindsight, poor.

So far, all of my tests have needed to run in the browser. Since I have multiple classes in ICE, I have been creating a new file to test each of these classes. I think that is OK, but what is not OK—at least not when there is certain kinds of js-interop work involved—is how I included the tests. On the web page that provided the test suite context, I had included the various test files as:
<head>
  <title>ICE Test Suite</title>
  <script type="application/dart" src="editor_test.dart"></script>
  <script type="application/dart" src="store_test.dart"></script>
  <script type="application/dart" src="gzip_test.dart"></script>
  <script type="application/dart" src="full_test.dart"></script>
</head>
Each of those _test.dart files had their own main() entry point. For normal Dart code (and even certain classes of tests), this is perfectly OK. What makes it not OK is that the editor.dart pulled JS into its own isolate:
import 'package:unittest/unittest.dart';
import 'dart:html';
import 'package:ice_code_editor/ice.dart';

main() {
  // tests that create js-interop proxies
}
And then the full_test.dart did the exact same thing:
import 'package:unittest/unittest.dart';
import 'dart:html';
import 'package:ice_code_editor/ice.dart';

main() {
  // tests that create js-interop proxies
}
So both tests have their own main isolate, but share libraries and ultimately try to share js-interop proxies into the same JavaScript code (the ACE code editor in this case). The first time through, everything is OK. The second time through, js-interop tries to setup the first proxy in the new isolate, only to find that it has already started a zeroeth proxy. Even if that succeeds (and I'm not sure how it could), there is still a problem caused by double-loading ACE JavaScript script files that result in the cannot read property 'ace/ace' of undefined. Some of what Damon and I did last night ensured that the ACE JavaScript source would only load once. But obviously that only works in a single isolate.

So it seems clear that I have no choice but to run all of my tests in the same main() isolate. If I want to retain the same separation of testing concerns, this means that I need to move my test files into Dart “parts.”

Starting from the top, I load a single (new) ice_test.dart source file into my testing web page context:
<head>
  <title>ICE Test Suite</title>
  <script type="application/dart" src="ice_test.dart"></script>
  <script src="packages/browser/dart.js"></script>
</head>
In ice_test.dart, I import the packages necessary to run all of my tests: unittest, dart:html and, of course, ice. Then I declare the various parts that make up this testing library:
library ice_test;

import 'package:unittest/unittest.dart';
import 'package:ice_code_editor/ice.dart';
import 'dart:html';

part 'editor_test.dart';
part 'store_test.dart';
part 'gzip_test.dart';
part 'full_test.dart';

main(){
  editor_tests();
  store_tests();
  gzip_tests();
  full_tests();
}
I have to give separate names to each of the functions that run the tests in editor_test.dart, full_test.dart, etc. so that the names do not clash. I start with the convention of using the plural of the file name.

The last thing that I need to do is declare each of the _test.dart files as parts of the ice_test library. The editor_test.dart file then becomes:
part of ice_test;

editor_tests() {
  // test here
}
After doing the same in the other test files, I am ready to run my test suite and... it works!



Yay! No red. All in all, I am pretty happy with that solution. I might prefer not to have to follow the convention of popularizing the the test functions, but if that is the only bother that I have in my test organization, I can live with it.


Day #752

Tuesday, May 14, 2013

Better DOM Tests in Dart

‹prev | My Chain | next›

Last night I and Santiago Arias (my #pairwithme pair) made a decent start on the full-screen, mini-IDE version of the Dart-flavor ICE Code Editor. We are finding that the test suite is growing messy—primarily, I suspect, due to the manner in which all of the tests run asynchronously (i.e. at once). With so many elements all creating an instance of the ICE Code Editor and tearing down the DOM elements used to build it, tests are stomping on each other. Tonight, I hope to fix that up a bit.

Dart provides facilities to run groups of tests serially, but I do not think that I need make use of that—or at least I think that I can address most of my problems without doing that. To illustrate the point, most of the errors are along the lines of:
TypeError {} ace.js:5464
Could not load worker ace.js:5463
TypeError {} ace.js:5464
Uncaught TypeError: Cannot read property 'ace/ace' of undefined ace.js:114
Could not load worker ace.js:5463
TypeError {} ace.js:5464
Uncaught TypeError: Cannot read property 'ace/ace' of undefined ace.js:114
Could not load worker ace.js:5463
TypeError {} 
My initial thought is that my setup and teardown are being overwritten by async tests all writing to the <div> tag with the same ID:
  group("content", () {
    var el;
    setUp(() {
      el = new Element.html('<div id=ice>'); document.body.nodes.remove(el));

    test("can set the content", () {
      var it = new Editor('#ice');
      // ...
    });
  });
There are more tests that do the same, so I try creating unique IDs using the test-case ID:
group("content", () {
    setUp(() {
      var el = new Element.html('<div id=ice-${currentTestCase.id}>');
      document.body.nodes.add(el);
    });
    tearDown(() {
      var el = document.query('#ice-${currentTestCase.id}');
      document.body.nodes.remove(el);
    });

    test("can set the content", () {
      var it = new Editor('#ice-${currentTestCase.id}');
      // ...
    });
  });
If I am right, then using unique <div> tags ought to clean up the test cases.

It turns out that I am wrong. I still see the same errors. So instead, I work with Damon Douglas, tonight's #pairwithme pair, to get to the bottom of this. The bottom turns out to be the ACE JavaScript editor that is being loaded via js-interop. Loading ACE once is fine. Loading it several times as in test cases: not so fine.

The solution is to move the dynamic creation of the <script> tags for the ACE editor into a one-time only static method in the ICE.Editor class:
  static _attachScripts() {
    if (_isAceJsAttached) return;

    var script_paths = [ /* ... */ ];

    var scripts = script_paths.
      map((path) {
        var script = new ScriptElement()
          ..async = false
          ..src = path;
        document.head.nodes.add(script);
        return script;
      }).
      toList();

    return _scripts = scripts;
  }
If the private method indicating that the ACE JavaScript is already loaded returns true, then there is no need to dynamically create new <script> elements that load ACE again and again.

I really seem to be paying the price for dynamically creating those ACE <script> tags. It was a pain to get working in the first place and it is a pain to be able to test with them in the way. And yet, I cannot bring myself to get rid of them. From the outside world, a developer should rarely need to worry about this. Even if it is a problem, there is a single Future, editorReady, that completes when everything is in place. All of this makes for a web page that contains no more than:
<head>
  <script src="packages/browser/dart.js"></script>
  <script type="application/dart">
    import 'package:ice_code_editor/ice.dart' as ICE;
    main()=> new ICE.Full();
  </script>
</head>
With just that, all of the JavaScript is loaded, along with the Dart that controls it. This still seems a worthy goal. And, since I have my tests in better shape thanks to Damon, I will stick to this path for a little while longer.


Day #751