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: correctly re-establishes pipe destinations #2582

Merged
merged 5 commits into from
Oct 20, 2020

Conversation

reggi
Copy link
Contributor

@reggi reggi commented Oct 14, 2020

NODE-2172

I tried pretty hard to recreate this locally, and I ran into issues, I was also considering stubbing out the #pipe here and testing the createChangeStreamCursor function directly but that would mean exporting it which would make it public. I can't get this code to fire and it does not exist in master anymore.

It does seem verifiably wrong.

const x = ['pipe1', 'pipe2', 'pipe3']
> for (let pipe in x) { console.log(pipe) }
0
1
2
> for (let pipe of x) { console.log(pipe) }
pipe1
pipe2
pipe3

@reggi reggi requested review from mbroadst, emadum and nbbeeken and removed request for mbroadst October 15, 2020 16:43
@mbroadst
Copy link
Member

@reggi I think this ticket was about reenabling this test. Looks like you found a bug here, but were you able to get that test passing? Are there other tests we should include here to validate your fix?

@reggi
Copy link
Contributor Author

reggi commented Oct 16, 2020

@reggi I think this ticket was about reenabling this test. Looks like you found a bug here, but were you able to get that test passing? Are there other tests we should include here to validate your fix?

@mbroadst good learning here to do a search for the jira ticket in the codebase (which I don't normally do). I fixed the test, but in 3.6 it wasn't broken because of this pipe issue, it was the file /tmp/ dir not existing. Fixed this by setting it to a already existing dir, then cleaning up the file using this.defer, master may show different issues.

In the description I wrote about attempting to test this in => of switch, and I had spent some time on different solutions and came up blank. Any ideas? Is it necessary? This code is not in master anymore. It might not be worth it.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

This fix LGTM for 3.6. Just to clarify, we also need to get this functionality working again in master. That might be a little more complicated, since we've pulled the Readable stream off the Cursor object. There are currently two skipped tests in master - the one you fixed here, and a new somewhat simplified one I wrote. We should try to get at least one of them passing.

@reggi reggi merged commit a6e7caf into 3.6 Oct 20, 2020
@reggi reggi deleted the NODE-2172/3.6/pipe-destination branch October 20, 2020 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants