Skip to content
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

Fix debounceEvent #9186

Merged
merged 3 commits into from
Jul 14, 2016
Merged

Fix debounceEvent #9186

merged 3 commits into from
Jul 14, 2016

Conversation

f111fei
Copy link
Contributor

@f111fei f111fei commented Jul 13, 2016

debounceEvent is abnormal sometimes.

Steps to Reproduce:

Here is my test case:

test('Debounce Event', function () {
    let doc = new Samples.Document3();

    let onDocDidChange = debounceEvent(doc.onDidChange, (prev: string[], cur) => {
        if (!prev) {
            prev = [cur];
        } else if (prev.indexOf(cur) < 0) {
            prev.push(cur);
        }
        return prev;
    }, 10);

    onDocDidChange(keys => {
        assert.ok(keys, 'was not expecting keys.');
        if (keys.length === 3) {
            doc.setText('4');
            assert.deepEqual(keys, ['1', '2', '3']);
        } else {
            assert.deepEqual(keys, ['4']);
        }
    });

    doc.setText('1');
    doc.setText('2');
    doc.setText('3');
}); 
  1. Insert test code to event.test.ts.
  2. run scripts\test.

Here is error:
was not expecting keys.

Reason:
Some code is changing output during emitter.fire

Solution:
Reset output before emitter.fire

@jrieken
Copy link
Member

jrieken commented Jul 13, 2016

Some code is changing output during emitter.fire

not sure why you mean by that? Do you have two listeners and one modifies the event object?

@jrieken
Copy link
Member

jrieken commented Jul 13, 2016

Oh, I see - you trigger event while processing them... Can you pls add you test to this PR

@jrieken jrieken added this to the July 2016 milestone Jul 13, 2016
@f111fei
Copy link
Contributor Author

f111fei commented Jul 13, 2016

@jrieken it's here bec3066

@@ -177,6 +177,33 @@ suite('Event',function(){
Errors.setUnexpectedErrorHandler(origErrorHandler);
}
});

test('Debounce Event', function () {
Copy link
Member

Choose a reason for hiding this comment

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

since this test is async (setTimeout) you must add the done parameter and call it. Alternatively make this test return a promise.

@f111fei
Copy link
Contributor Author

f111fei commented Jul 13, 2016

@jrieken it's ok? a7f47fa

@jrieken
Copy link
Member

jrieken commented Jul 13, 2016

yeah - looks good. waiting for the builds to finish

@jrieken
Copy link
Member

jrieken commented Jul 13, 2016

@f111fei - sorry we had some hiccups today. Can you rebase with master and try again?

@f111fei
Copy link
Contributor Author

f111fei commented Jul 14, 2016

@jrieken I do as you say but I'm not sure if I'm right.

@jrieken
Copy link
Member

jrieken commented Jul 14, 2016

LGTM - thanks for the contribution

@jrieken jrieken merged commit a7dfdde into microsoft:master Jul 14, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

None yet

3 participants