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.

Sign up for GitHub

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

Jump to bottom

fix: [botframework-streaming] Some named pipe tests are not passing after fixing their inconclusiveness #4494

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: [botframework-streaming] Some named pipe tests are not passing after fixing their inconclusiveness #4494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a37c271
fix tests
erquirogasw Jun 13, 2023
4e09988
suggested changes applied
erquirogasw Jun 13, 2023
1e624bf
Add suggested changes
erquirogasw Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ export class NamedPipeTransport implements ITransportSender, ITransportReceiver
* @returns The buffer containing the data from the transport.
*/
receive(count: number): Promise<INodeBuffer> {
if (this._activeReceiveResolve) {
if (!this.socket) {
throw new Error('Cannot receive data over an unavailable/null socket.');
} else if (this.socket.destroyed) {
throw new Error('Cannot receive data over a dead/destroyed socket.');
} else if (this._activeReceiveResolve) {
throw new Error('Cannot call receive more than once before it has returned.');
}

Expand Down
128 changes: 92 additions & 36 deletions libraries/botframework-streaming/tests/NamedPipe.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
const assert = require('assert');
const { expect } = require('chai');
const { expectEventually } = require('./helpers/expectEventually');
const { NamedPipeClient, NamedPipeServer, StreamingRequest } = require('../lib');
const { NamedPipeClient, NamedPipeServer, StreamingRequest, StreamingResponse } = require('../lib');
const { NamedPipeTransport } = require('../lib/namedPipe');
const { platform } = require('os');
const { RequestHandler } = require('../lib');
const { createNodeServer, getServerFactory } = require('../lib/utilities/createNodeServer');

class FauxSock {
constructor(contentString) {
if (contentString) {
Expand Down Expand Up @@ -166,20 +164,31 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function ()
expect(() => transport.close()).to.not.throw();
});

// TODO: 2023-04-24 [hawo] #4462 The code today does not allows the receive() call to be rejected by reading a dead socket.
// The receive() call will be rejected IFF the socket is closed/error AFTER the receive() call.
it.skip('throws when reading from a dead socket', async function () {
it('throws when reading from a dead socket', async function () {
const sock = new FauxSock();
sock.destroyed = true;
sock.connecting = false;
sock.writable = true;
const transport = new NamedPipeTransport(sock, 'fakeSocket5');
expect(transport).to.be.instanceOf(NamedPipeTransport);
expect(transport.isConnected).to.be.false;
(await expectEventually(transport.receive(5))).to.throw();
expect(() => transport.receive(5)).to.throw('Cannot receive data over a dead/destroyed socket.');
expect(() => transport.close()).to.not.throw();
});

it('throws when reading from a unavailable/null socket', async function () {
const sock = new FauxSock();
sock.destroyed = false;
sock.connecting = false;
sock.writable = true;
const transport = new NamedPipeTransport(sock, 'fakeSocket5');
expect(transport).to.be.instanceOf(NamedPipeTransport);
expect(() => transport.close()).to.not.throw();
expect(transport.isConnected).to.be.false;
expect(transport.socket).to.be.null;
expect(() => transport.receive(5)).to.throw('Cannot receive data over an unavailable/null socket.');
});

it('can read from the socket', function () {
const sock = new FauxSock();
sock.destroyed = false;
Expand Down Expand Up @@ -258,20 +267,44 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function ()
expect(() => client.disconnect()).to.not.throw();
});

// TODO: 2023-04-24 [hawo] #4462 The client.send() call will only resolve when the other side responded.
// Because the other side is not connected to anything, thus, no response is received.
// Thus, the Promise is not resolved.
it.skip('sends without throwing', function (done) {
const req = new StreamingRequest();
req.Verb = 'POST';
req.Path = 'some/path';
req.setBody('Hello World!');
client
.send(req)
.catch((err) => {
expect(err).to.be.undefined;
})
.then(done);
it('sends without throwing', function (done) {
const response = new StreamingResponse();
response.statusCode = 111;
response.setBody('Test body.');
class TestRequestHandler extends RequestHandler {
constructor() {
super();
}
processRequest(_request, _logger) {
return response;
}
}
const server = new NamedPipeServer('pipeClientTest', new TestRequestHandler());
const client = new NamedPipeClient('pipeClientTest');
const pingServer = () =>
new Promise((resolve) => {
// Ping server before sending any information, so the server makes the connection over the socket.
client.send(new StreamingRequest());
const interval = setInterval(() => {
if (server.isConnected) {
clearInterval(interval);
resolve();
}
}, 100);
});
try {
server.start(async () => {
await client.connect();
await pingServer();
const result = await client.send(new StreamingRequest());
expect(result.statusCode).to.equal(response.statusCode);
server.disconnect();
client.disconnect();
done();
});
} catch (err) {
done(err);
}
});
});

Expand Down Expand Up @@ -310,21 +343,44 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function ()
expect(server.isConnected).to.be.true;
});

// TODO: 2023-04-24 [hawo] #4462 The client.send() call will only resolve when the other side responded.
// Because the other side is not connected to anything, thus, no response is received.
// Thus, the Promise is not resolved.
it.skip('sends without throwing', function (done) {
const server = new NamedPipeServer('pipeA', new RequestHandler());
expect(server).to.be.instanceOf(NamedPipeServer);
expect(() => server.start()).to.not.throw();
const req = { verb: 'POST', path: '/api/messages', streams: [] };
server
.send(req)
.catch((err) => {
expect(err).to.be.undefined;
})
.then(expect(() => server.disconnect()).to.not.throw())
.then(done);
it('sends without throwing', function (done) {
const response = new StreamingResponse();
response.statusCode = 111;
response.setBody('Test body.');
class TestRequestHandler extends RequestHandler {
constructor() {
super();
}
processRequest(_request, _logger) {
return response;
}
}
const server = new NamedPipeServer('pipeServerTest');
const client = new NamedPipeClient('pipeServerTest', new TestRequestHandler());
const pingServer = () =>
new Promise((resolve) => {
// Ping server before sending any information, so the server makes the connection over the socket.
client.send(new StreamingRequest());
const interval = setInterval(() => {
if (server.isConnected) {
clearInterval(interval);
resolve();
}
}, 100);
});
try {
server.start(async () => {
await client.connect();
await pingServer();
const result = await server.send(new StreamingRequest());
expect(result.statusCode).to.equal(response.statusCode);
server.disconnect();
client.disconnect();
done();
});
} catch (err) {
done(err);
}
});

it('handles being disconnected', function () {
Expand Down
Loading