Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

More Editor Option Handlers Tests #3231

Merged
merged 8 commits into from
Apr 7, 2013
Merged

More Editor Option Handlers Tests #3231

merged 8 commits into from
Apr 7, 2013

Conversation

TomMalbran
Copy link
Contributor

I added tests for show line numbers and auto close brackets and refactored the code to reduce the code repetition. The difference is all wrong, since I added new indentation and new helper functions, but the functionality is all the same as before for the older tests.


if (oEvent.initKeyboardEvent) {
oEvent.initKeyboardEvent(event, true, true, doc.defaultView, false, false, false, false, key, key);
oEvent.initKeyboardEvent(event, true, true, doc.defaultView, key, 0, false, false, false, false);
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameters where passed in the wrong order here: http://stackoverflow.com/questions/12955779/how-can-i-add-a-keycode-to-a-synthetic-keyboardevent-in-webkit.

The charCode part was needed to be used in Strings.fromCharCode in CodeMirror.

@ghost ghost assigned jbalsas Apr 4, 2013
@jbalsas
Copy link
Contributor

jbalsas commented Apr 4, 2013

Reviewing

@@ -62,11 +71,20 @@ define(function (require, exports, module) {
SpecRunnerUtils.closeTestWindow();
});

function checkLineWrapping(firstPos, secondPos, shouldWrap, inlineEditor) {
function getEditor(isInlineEditor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really fond of this method, and the consequences to the rest of methods' parameter lists... As we're only testing one editor at a time, I'd prefer if we could tweak openEditor and openInlineEditor so that at the end, and before the checks, it would be pointing to the right editor (full or inline).

However, unless you can do that quickly, it's not probably worth the effort, so don't worry too much about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do keep it this way, I'd probably call the parameter just inline as the method getEditor already gives away the rest ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea, and I have it implemented and working. Will push it with the rest of the review fixes, once the review is done :)

editor,
lineInfo;

describe("Toggle Auto Close Brackets", function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature is actually closed Auto Close Braces, and also works for other chars such as "", '', (), {}. We should change the suite name.

It's probably not critical at all, as I'd assume if it works for one, it should for the others, but if it's not too much complicated to add those additional checks (pass an array of chars to checkCloseBrackets, for example?, or just do several checks in the tests...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking the codemirror addon and I don't think it's worth the extra noise to add those other checks for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed all the names already.

I figure if it worked for 1, it should work for all, since every key is added to the key map and assigned the same function making all of them work in the same way. Anyway, maybe 1 extra test just to check that it works for all the braces, might be useful?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, imho. Would probably be just meaningless tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing one though, for auto-close in inline editors

@jbalsas
Copy link
Contributor

jbalsas commented Apr 5, 2013

@TomMalbran Done with review. Looks good overall, and nice refactors out there ;)

I'll have some sleep time, so don't go rushing to fix everything!

@TomMalbran
Copy link
Contributor Author

@jbalsas Thanks again for the review! I think I covered all your comments.

I'll need some explaining here, as I'd expect to have var myContent = "This is awesome!";[ after typing [ with auto-close-braces off. Is this because of how codemirror or the addon handles the keypresses?

I am not sure why it doesn't add the key. I do know that the function that handles the keypresses in CodeMirror searches through the keymaps and applies the function that it finds, so that makes the auto close braces work, but I do not know why it does nothing when it doesn't find a function. I still haven't find how CodeMirror handles the inputs, and I did tried several simulation and this is the one that works better.


expect(editor).toBeTruthy();
gutterElement = editor._codeMirror.getGutterElement();
$gutter = testWindow.$(gutterElement).find(".CodeMirror-linenumbers");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just $(gutterElement).find(".CodeMirror-linenumbers") ?

Also, $gutter seems somehow misleading. How about $lineNumbers?

@jbalsas
Copy link
Contributor

jbalsas commented Apr 5, 2013

@TomMalbran Re-review done, just two minor comments and we can merge this.

I'm still not happy that strange missing bracket character issue... I'll try to spend some time on if afterwards.

});

afterEach(function () {
SpecRunnerUtils.closeTestWindow();
});

function checkLineWrapping(firstPos, secondPos, shouldWrap, inlineEditor) {

function checkLineWrapping(firstPos, secondPos, shouldWrap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment, if you're up to it.

I think all these helper check methods should receive as a first parameter the editor to check against. This way, the helper methods are agnostic of what happens on the outside, and will be the test the responsible for deciding which editor to check. I think this is a bit more robust than just relying on the editor being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is kind of how it was working before with the isInlineEditor parameter and using the getEditor function. But to implement it with the new code, the open functions could return the editor instead of using the global variable, and then then send that to the check function. Would that work better? It might look better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ideally the openEditor functions would return a promise that would resolve with the opened editor. However, that will add lots of code and make the tests less readable. You can try, though, to see if there's an easy way, but I think we can compromise for now in at least passing the editor to the helper methods so that those remain independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried passing the editor as the first parameter but it still doesn't work, I believe that the problem is that the passed editor hasn't been resolved yet, so when the run function from the check functions work, they will use the undefined editor. Anyway, since the test already calls any of the open editor functions (depending on the test made) and since the editor variable gets nulled after each test, it already decides which editor to use without having to explicitly passed as a parameter, and since everything is run inside the run function it should work synchronously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this can be a bit tricky with all the runs and waitsFor...... another option would be to use your previous getEditor method inside the tests to resolve the editor variable, and then pass it to the helper methods.

What do you think? Could you give that a try? Otherwise, we can leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should put the runs around the helper method call (inside the it clause). That way, all promises would have resolved, and you can get the proper editor and pass it to the check methods. Also, the runs in the check methods should now not be needed anymore, as we'd be waiting for the promises to resolve outside them.

I'd suggest something like:

function checkLineWrapping(editor, ...) {
   //expectSomethingFromEditor
}

it("should do something", function () {
  openEditor(HTML_FILE);
  runs(function() {
    var editor = getFullEditor(),
        inlineEditor = getInlineEditor(0);

    checkLineWrapping(editor, ...);
    checkLineWrapping(inlineEditor, ...);
  }
});

This gives us also the possibility of easily testing conditions throughout several editors (full and inlines) at the same time.

What do you think? Could you check this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't see your edited comment.

Is what I just proposed what you had before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is one of the way it worked if we want to pass the editor as a parameter. The other option was using a deferred and run the code inside the a done function from the open editor instead of a run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think passing the editor as a parameter gives us better control, and keeps the helper methods API clean, but at this point I feel like we're dancing around for some little gain.

What do you think? Do you feel more strongly about some of the other options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runs would be ok to add to each function. I'll add those.

@TomMalbran
Copy link
Contributor Author

I think passing the editor as a parameter gives us better control, and keeps the helper methods API clean, but at this point I feel like we're dancing around for some little gain.

Done, and since all the test were running a bit slowing as they had to open an close several windows, I managed to make them run all in the same window, which makes it faster and nicer, but let me know if there is any problem with this.

I'm still not happy that strange missing bracket character issue... I'll try to spend some time on if afterwards.

I think the problem here is not with the code but is the fact that the event simulation creates a new event that can be handled with a listener but won't make a key input in a textarea or input field. So the text isn't added to the textarea that CodeMirror uses, but CodeMirror does listen to the event and calls the addon to it's job.

@jbalsas
Copy link
Contributor

jbalsas commented Apr 7, 2013

Thanks for working on this. Some of the runs could probably be removed, but we could address that in a later cleanup if desired.

Looks good! Merging.

jbalsas added a commit that referenced this pull request Apr 7, 2013
@jbalsas jbalsas merged commit d2cf66e into adobe:master Apr 7, 2013
@TomMalbran
Copy link
Contributor Author

Great, thanks.

Which runs are you referring to? If is the ones that check the current selection, I tried removing them, but couldn't, since it was getting executed before the check functions where run and it then expected 0 to be the given position.

@TomMalbran TomMalbran deleted the tom/editor-options-tests branch April 7, 2013 10:45
@jbalsas
Copy link
Contributor

jbalsas commented Apr 7, 2013

I was thinking of the ones inside the check... methods, since nothing should have async issues in there anymore, but it's just a minor nit.

@TomMalbran
Copy link
Contributor Author

I see. I am planning on add an option to toggle the auto close tags, and add tests, so I might add it there if everything works without them.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants