-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
entering values and triggering events in tests #5
base: master
Are you sure you want to change the base?
Conversation
Progress, but not 100%; will continue working tonight but have to catch a plane now. |
Have a safe 🛩 |
Thanks -- got a lot of this working, and I am now near the point of not being sure if I'm failing to write a test or the test is failing. Especially on the first line each of |
They look fine at a glance. Don't really have time to go deep into them now, but integration tests are a fragile thing. You'll probably need a bit of |
text entry using runCommand autocompletion click, event listeners, but callback hell refactored callback hell basic usage tested
932c471
to
936d1a1
Compare
I just got this working, and think it's close to ready for merge, or perhaps ready if you agree. I'm going to move the last two items into a separate pull request. There's just one oddity left -- I had to run a |
Changed my mind -- testing multiple modes made me refactor some of this code, and it made enough sense to submit it together. I also have a question -- the last test is failing because a trailing space is inserted in markdown mode that I didn't expect -- but the space doesn't seem to be inserted in wysiwyg mode. Which is the intended behavior?
|
I think the extra space may be this I see how inserting an extra space makes sense, but it's odd that it's not reflected in wysiwyg mode. Maybe the contentEditable doesn't return it somehow, or maybe wysiwyg mode runs an extra trim operation? |
expect(editor.value()).toEqual('hello wysiwyg @hodor'); | ||
|
||
editor.runCommand(function(chunks, mode) { | ||
console.log(chunks,"expect insertion point to be at the end"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bevacqua - I'm circling back after a few weeks of other work, and I've tracked things to here. Using console.log
I can see that chunks.before
and after
are correct within banksy's custom appendHTML()
function. But by the time I test them here (triggered by the crossvent.fabricate
on line 111), the insertion point has moved to the beginning of the text, and chunks.before
and after
have changed. Any idea on when/where/how this might be happening? Maybe some kind of focus/refocus event in the midst of user interaction? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've followed chunks
all the way to writeSelectionEditable
in InputState
and getSurface
, and they keep the input text in chunks.before
late in the process as I can follow them, it seems. Only when getChunks()
is called can I console.log
out a chunks
state where the inputted text has been moved to chunks.after
.
I can also visually confirm that the insertion point is moved to the beginning by interacting manually with the wysiwyg mode in the open test window, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progress... I found that InputHistory.setCommandMode
here is run, triggering InputHistory.refreshState
, here, where I believe the InputState's start
and end
properties are overwritten. This happens exactly where the chunks.before
and chunks.after
become incorrect, and this relates to history. I want to find what's triggering setCommandMode
, or why refreshState
is overwriting the cursor position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also see that here, setCommandMode
(which runs a little before the test fails) can trigger refreshState, but does so within a setTimeout
command, so asynchronously. Since the test (as currently written) also uses setTimeout (to get the events to sequence correctly), I wonder if this setCommandMode
call is actually getting asynchronously executed in the middle of the test.
I'm going to look at crossvent and see if we can remove the setTimeout
usage from the test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, removing the setTimeout
seems actually to allow the chunks
object to retain the correct values, but causes the click
event not to trigger properly, which is why I'd added it in the first place. @bevacqua - I'm really sorry to bother you, but can you offer any insight into how I might get the click
event to trigger in this test without using a setTimeout(function() {}, 0)
? I think if I can get that to work, I can clean up and complete this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the before/after swap issue I'm chasing may not be an artifact of the timing of the tests themselves. I'm seeing the behavior also when manually entering hi @h
and autocompleting, which you can try here: https://publiclab.org/editor/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking into this again, @bevacqua -- I'm thinking that I need to perhaps set aside the integration tests for now and examine some of the lower, unit-level assumptions, maybe in horsey
before I can crack this. Horsey's v4.x doesn't have a horse.anchor
property, though, so it's changed a good bit between v3 and v4. Are you planning on maintaining banksy
to support horsey
v4 and forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really had the time to work on this one for a while. I'll collaborate with you in any way I can though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Egads! I worked through the changes in the horsey
API for v3/v4, and the insertion point shift bug is no longer occurring in manual use. (!!)
I'm going to try refactoring these tests for horsey
v4, and (not to jinx it), maybe these tests will pass? Whoa, sudden excitement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting a little bogged down converting to v4, but making progress. Horsey's triggering of setMode
is within a setTimeout
deferred method, and I'm having trouble getting my tests to run after a mode change. But I think it's going to be more rewarding than writing the test suite against v3 of horsey
, long term, and I was basically stuck, anyhow.
In manual use, too, I am seeing improper ordering of the crossvent.add
listeners due to the deferment of setMode
in woofmark
. A bit of a thicket and I've asked on ponyfoo slack for some tips on this kind of code.
This pull request isn't ready -- it's a placeholder.
It will incorporate event triggering to cause a horsey to appear, to lay the groundwork for horsey testing. No animals harmed, etc.
enter()
test method (instead of horsey appearances usingeditor.value()
).is(':visible')
test inconsistency -- is the show/hide triggering asynchronous or something?banksy.js:84 Uncaught TypeError: Cannot read property 'before' of undefined
on triggering click on menu using jQuery. Possibly switch to a crossvent event.