Skip to content

Commit

Permalink
refactor(test-tooling): fix types of streams: use NodeJS.ReadableStream
Browse files Browse the repository at this point in the history
1. The container management library that we use in the test infrastructure
(called dockerode) is expecting streams that are defined in the global
namespace of the `@types/node` library, e.g. the standard library of NodeJS
itself.
2. Previously we were using the "streams" package to provide type information
to the streams that we were passing around to dockerode and it was working
fine, but after some changes that seem unrelated this has broken the
compilation process.
3. The mentioned changes are not yet on the main branch, but we expect
them to be there soon and so this change is laying the groundwork for that
by pre-emptively fixing the broken build's root cause which is that the
test-tooling package does not declare it's typings related dependencies
correctly: It implicitly uses the NodeJS standard library's types but
so far had not declared them on the package level.
4. This change is therefore to rectify the issue of the `@types/node`
dependency missing from the test-tooling package and also the refactoring
of some of the test ledger classes which were relying on the `streams`
builtin package instead of correctly using the NodeJS.ReadableStream global.
5. Earlier the reasoning for this was that we try to avoid pulling in
types from the global scope because we try to avoid any sort of dependency
on the global scope in general. Once we have proof though that this is
causing issues with the build, then we must give up the principle for
practical reasons (and only in the minimum viable scope, e.g. this does
not change the fact that everywhere else in the codebase we should still
do our best to avoid using the global scoped classes, types, functions,
etc..).

Thank you to @AndreAugusto11 and @RafaelAPB for pointing out this issue
through the pull request of his that is currently being worked on at the
time of this writing:
RafaelAPB#72

Related to but does not address hyperledger-cacti#2811

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
  • Loading branch information
petermetz committed Mar 4, 2024
1 parent 31c6464 commit 8139132
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 53 deletions.
1 change: 1 addition & 0 deletions packages/cactus-test-tooling/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
"@types/fs-extra": "9.0.13",
"@types/js-yaml": "4.0.3",
"@types/lodash": "4.14.172",
"@types/node": "20.11.24",
"@types/node-forge": "1.3.0",
"@types/ssh2": "0.5.47",
"@types/ssh2-streams": "0.1.9",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import path from "path";
import { Duplex, Stream } from "stream";
import { Duplex } from "stream";
import { IncomingMessage } from "http";
import throttle from "lodash/throttle";
import { Container, ContainerInfo } from "dockerode";
Expand Down Expand Up @@ -443,7 +443,10 @@ export class Containers {
log.debug(JSON.stringify(msg.progress || msg.status));
}, 1000);

const pullStreamStartedHandler = (pullError: unknown, stream: Stream) => {
const pullStreamStartedHandler = (
pullError: unknown,
stream: NodeJS.ReadableStream,
) => {
if (pullError) {
log.error(`Could not even start ${imageFqn} pull:`, pullError);
reject(pullError);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import isPortReachable from "is-port-reachable";
import Joi from "joi";
import { EventEmitter } from "events";
import { ITestLedger } from "../i-test-ledger";
import { Stream } from "stream";

const OPTS_SCHEMA: Joi.Schema = Joi.object().keys({
imageVersion: Joi.string().min(5).required(),
Expand Down Expand Up @@ -211,22 +210,25 @@ export class HttpEchoContainer implements ITestLedger {
private pullContainerImage(containerNameAndTag: string): Promise<unknown[]> {
return new Promise((resolve, reject) => {
const docker = new Docker();
docker.pull(containerNameAndTag, (pullError: unknown, stream: Stream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
});
docker.pull(
containerNameAndTag,
(pullError: unknown, stream: NodeJS.ReadableStream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
},
);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
import { ITestLedger } from "../i-test-ledger";
import { Streams } from "../common/streams";
import { Containers } from "../common/containers";
import { Stream } from "stream";

/*
* Contains options for Postgres container
Expand Down Expand Up @@ -287,22 +286,25 @@ export class PostgresTestContainer implements ITestLedger {
private pullContainerImage(containerNameAndTag: string): Promise<unknown[]> {
return new Promise((resolve, reject) => {
const docker = new Docker();
docker.pull(containerNameAndTag, (pullError: unknown, stream: Stream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
});
docker.pull(
containerNameAndTag,
(pullError: unknown, stream: NodeJS.ReadableStream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
},
);
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Stream } from "stream";
import { EventEmitter } from "events";
import axios from "axios";
import { v4 as uuidv4 } from "uuid";
Expand Down Expand Up @@ -415,22 +414,25 @@ export class QuorumTestLedger implements ITestLedger {
private pullContainerImage(containerNameAndTag: string): Promise<unknown[]> {
return new Promise((resolve, reject) => {
const docker = new Docker();
docker.pull(containerNameAndTag, (pullError: unknown, stream: Stream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
});
docker.pull(
containerNameAndTag,
(pullError: unknown, stream: NodeJS.ReadableStream) => {
if (pullError) {
reject(pullError);
} else {
docker.modem.followProgress(
stream,
(progressError: unknown, output: unknown[]) => {
if (progressError) {
reject(progressError);
} else {
resolve(output);
}
},
);
}
},
);
});
}

Expand Down
10 changes: 10 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9263,6 +9263,7 @@ __metadata:
"@types/fs-extra": "npm:9.0.13"
"@types/js-yaml": "npm:4.0.3"
"@types/lodash": "npm:4.14.172"
"@types/node": "npm:20.11.24"
"@types/node-forge": "npm:1.3.0"
"@types/ssh2": "npm:0.5.47"
"@types/ssh2-streams": "npm:0.1.9"
Expand Down Expand Up @@ -15411,6 +15412,15 @@ __metadata:
languageName: node
linkType: hard

"@types/node@npm:20.11.24":
version: 20.11.24
resolution: "@types/node@npm:20.11.24"
dependencies:
undici-types: "npm:~5.26.4"
checksum: 10/7f34bfae5f9b98b9910230af4b4c52dc7fb2d1e96fdebfbc3d7576f8ab3d100076f193f9469add9e7418b455294155e7e6a028498cc5e98f9d49349875a459cf
languageName: node
linkType: hard

"@types/node@npm:20.4.7":
version: 20.4.7
resolution: "@types/node@npm:20.4.7"
Expand Down

0 comments on commit 8139132

Please sign in to comment.