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 propagating event with secondary events #8

Conversation

rbrtribeiro
Copy link

Fix an issue with propagating the return value of an existing listener in order to avoid dumping

@rbrtribeiro rbrtribeiro force-pushed the bugfix/propagating-clientrequest-response branch from 4487005 to a245e29 Compare April 9, 2019 11:31
@@ -138,3 +139,33 @@ test('is able to propagate and map certain events', function(t) {

ee1.emit('event-1');
});

test('is able to propagate response from http.ClientRequest', function (t) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a maintainer on this package / repo, though we will need this fix for nock/nock#1485 so I wanted to weigh in.

In Nock, we've recently started regression-testing integration issues using smaller bracket tests which isolate the behavior we want Nock to have. These have replaced large-bracket integration tests which do reproduce the problem, but depend on behaviors and often the internals of other libraries, and don't make it clear to our developer specifically what the test is there to ensure. We've found the smaller-bracket tests are more understandable and make the library easier to maintain.

Would it be possible to create a test which isolates the behavior described in nock/nock#1485 (comment)?

});

response.on('close', () => {
t.notEqual(retrievedData, "");
Copy link
Member

Choose a reason for hiding this comment

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

There's a failing test in CI and I think adding a t.end() here will fix it.

@paulmelnikow
Copy link
Member

Wanted to let you know this project is now being maintained by the Nock team!

paulmelnikow added a commit that referenced this pull request Apr 12, 2019
Thanks to @rbrtribeiro for the fix! This rebases the work from #8 and tests the return values directly (rather than adding a wide-bracket integration test).

Close #8 Close #9
paulmelnikow added a commit that referenced this pull request Apr 12, 2019
Thanks to @rbrtribeiro for the fix! This rebases the work from #8 with minor modifications, and tests the return values directly (rather than adding a wide-bracket integration test).

Close #8 Close #9
paulmelnikow added a commit that referenced this pull request Apr 12, 2019
Thanks to @rbrtribeiro for the fix! This rebases the work from #8 with minor modifications, and tests the return values directly (rather than adding a wide-bracket integration test).

Close #8 Close #9
@paulmelnikow
Copy link
Member

This needed to be rebased so I picked it up in #17.

paulmelnikow added a commit that referenced this pull request Apr 12, 2019
This rebases the work from #8 with minor modifications, and tests the return values directly (rather than adding a wide-bracket integration test).

Adopts arrow functions in the test file.

This was identified as the root cause of nock/nock#1485.

Thanks to @rbrtribeiro for the fix! 

Close #8 Close #9
@nockbot
Copy link
Collaborator

nockbot commented Apr 12, 2019

🎉 This issue has been resolved in version 2.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants