[IS-9/feat]: as a library user i want to create the environment in order to allocate resources for the connections #12

l4mby · 2025-05-27T10:13:29Z

This PR introduces the possibility of Environment creation, and a first approach in the creation and deletion of RabbitMQ AMQP 1.0 connections.
We also adjusted e2e tests and introduced exploration tests for rhea the library which implements AMQP 1.0

Copilot

Pull Request Overview

This PR introduces an environment abstraction to manage AMQP connections and updates associated tests to verify connection allocation and resource management. Key changes include:

New unit tests for connection functionalities.
Implementation of a new Environment interface and its AmqpEnvironment class in src/environment.ts.
Adjustments to e2e tests and updates to RabbitMQ image versions in docker-compose and GitHub workflows.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/unit/rhea/connection.test.ts	Adds unit tests to validate connection creation via rhea.
test/support/util.ts	Introduces utility functions, including an eventually helper.
test/e2e/management.test.ts	Modifies test skipping and enabling for management queue creation.
test/e2e/environment.test.ts	Adds an end-to-end test for the environment abstraction.
src/environment.ts	Implements the Environment interface and connection management.
src/connection.ts	Provides a wrapper for AMQP connections with proper close handling.
docker-compose.yml	Updates RabbitMQ image version used in local development.
.github/workflows/main.yml	Updates RabbitMQ image version in CI environment.

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (2)

docker-compose.yml:3

[nitpick] Verify that downgrading the RabbitMQ image version is intentional, as it might impact compatibility.

image: rabbitmq:4.0.9-management

.github/workflows/main.yml:23

[nitpick] Ensure that the version change of the RabbitMQ image in workflows is intentional and consistent with the docker-compose configuration.

image: rabbitmq:4.0.9-management

Copilot · 2025-05-27T13:11:28Z

test/support/util.ts

+    } catch (error) {
+      if (elapsedFrom(start) > timeout) {
+        if (error instanceof AssertionError) throw error
+        expect.fail(error as string)


Consider converting the error to a descriptive string using error.message or String(error) to provide better error details.

Suggested change

expect.fail(error as string)

expect.fail(error instanceof Error ? error.message : String(error))

Copilot · 2025-05-27T13:11:28Z

src/environment.ts

+      })
+      this.container.once(ConnectionEvents.error, (context) => {
+        console.log(context)
+        rej()


Rejecting without a descriptive error can hinder debugging; include the error detail in the rejection.

Suggested change

rej()

rej(context.error || new Error("Connection error occurred"))

Copilot · 2025-05-27T13:11:29Z

src/connection.ts

+      })
+      this.rheaConnection.once(ConnectionEvents.connectionError, (context) => {
+        console.error("ERROR: ", context.connection.error)
+        return rej(false)


Rejecting the promise with a boolean value loses error context; consider rejecting with an informative error message.

Suggested change

return rej(false)

return rej(new Error("Connection error: " + context.connection.error))

gpad

Follow the comment of Copilito and put some description in the PR

Copilot

Pull Request Overview

This PR adds an Environment abstraction for managing AMQP connections, implements creation and cleanup of Rhea-based connections, and updates tests and Docker/RabbitMQ configurations accordingly.

Introduce AmqpEnvironment and createEnvironment for resource allocation.
Implement connection open/close logic in AmqpConnection and environment.
Update unit and E2E tests; pin RabbitMQ image to 4.0.9 and expose broker port.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/unit/rhea/connection.test.ts	Add unit tests for opening Rhea connections and links.
test/support/util.ts	Extend test utilities (HTTP calls, `eventually` helper).
test/e2e/management.test.ts	Toggle skipping of management E2E tests.
test/e2e/environment.test.ts	Add E2E test for new `Environment` API.
src/environment.ts	New `AmqpEnvironment` implementing `Environment` interface.
src/connection.ts	New `AmqpConnection` class wrapping Rhea connections.
docker-compose.yml	Downgrade RabbitMQ image to 4.0.9 and add port env var.
.github/workflows/main.yml	Align GitHub Actions RabbitMQ image version to 4.0.9.

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (1)

test/support/util.ts:4

AssertionError is not exported from assertion-error; import it from the built-in assert module (import { AssertionError } from "assert"), or use Chai’s AssertionError.

import { AssertionError } from "assertion-error"

Copilot · 2025-05-28T07:19:09Z

test/e2e/management.test.ts

 use(chaiAsPromised)

-describe("Management", () => {
+describe.skip("Management", () => {


The describe.skip call on the entire suite prevents all tests from running; if you intend to skip only specific tests, remove skip from the suite and apply it to individual test calls.

Suggested change

describe.skip("Management", () => {

describe("Management", () => {

Copilot · 2025-05-28T07:19:09Z

test/unit/rhea/connection.test.ts

+    container = create_container()
+  })
+
+  afterEach(async () => {})


[nitpick] Empty afterEach block means open connections aren't cleaned up between tests; consider closing the container or active connections to avoid resource leaks.

Suggested change

afterEach(async () => {})

afterEach(async () => {

try {

if (container) {

container.close(); // Close the container and its connections

}

} catch (error) {

console.error("Error during cleanup:", error);

}

})

Copilot · 2025-05-28T07:19:10Z

src/environment.ts

+  }
+
+  async close(): Promise<void> {
+    await this.closeConnections()


[nitpick] close clears the tracked connections but does not remove any event listeners from this.container; consider disposing or removing listeners to prevent memory leaks in long-lived processes.

Suggested change

await this.closeConnections()

await this.closeConnections()

this.cleanupListeners.forEach((cleanup) => cleanup())

this.cleanupListeners.length = 0

Copilot · 2025-05-28T07:19:10Z

docker-compose.yml

    environment:
      RABBITMQ_DEFAULT_USER: "rabbit"
      RABBITMQ_DEFAULT_PASS: "rabbit"
+      RABBITMQ_PORT: "5672"


RabbitMQ does not use RABBITMQ_PORT to expose the AMQP port; map ports in the ports: block or use the correct env var (RABBITMQ_NODE_PORT) to ensure the broker listens on the intended port.

Suggested change

RABBITMQ_PORT: "5672"

RABBITMQ_NODE_PORT: "5672"

Copilot · 2025-05-28T07:19:10Z

test/unit/rhea/connection.test.ts

+  return new Promise((res, rej) => {
+    connection.once("receiver_open", (context) => {
+      return res(context.receiver)
+    })
+    connection.once("receiver_error", (context) => {
+      return rej(context.connection.error)
+    })
+    connection.open_receiver({
+      snd_settle_mode: 1,
+      rcv_settle_mode: 0,
+      name: "management-link-pair",
+      target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },
+      source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },
+      properties: { paired: true },
+    })
+  })
+}
+
+async function openSender(connection: Connection) {
+  return new Promise((res, rej) => {
+    connection.once("sender_open", (context) => {
+      return res(context.sender)
+    })
+    connection.once("sender_error", (context) => {
+      return rej(context.connection.error)
+    })
+    connection.open_sender({
+      snd_settle_mode: 1,
+      rcv_settle_mode: 0,
+      name: "management-link-pair",
+      target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },
+      source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },
+      properties: { paired: true },
+    })


[nitpick] openReceiver and openSender share nearly identical promise/event wiring; consider extracting a helper to reduce duplication and simplify maintenance.

Suggested change

return new Promise((res, rej) => {

connection.once("receiver_open", (context) => {

return res(context.receiver)

})

connection.once("receiver_error", (context) => {

return rej(context.connection.error)

})

connection.open_receiver({

snd_settle_mode: 1,

rcv_settle_mode: 0,

name: "management-link-pair",

target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },

source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },

properties: { paired: true },

})

})

}

async function openSender(connection: Connection) {

return new Promise((res, rej) => {

connection.once("sender_open", (context) => {

return res(context.sender)

})

connection.once("sender_error", (context) => {

return rej(context.connection.error)

})

connection.open_sender({

snd_settle_mode: 1,

rcv_settle_mode: 0,

name: "management-link-pair",

target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },

source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },

properties: { paired: true },

})

return openLink(connection, "receiver_open", "receiver_error", connection.open_receiver.bind(connection), {

snd_settle_mode: 1,

rcv_settle_mode: 0,

name: "management-link-pair",

target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },

source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },

properties: { paired: true },

})

}

async function openSender(connection: Connection) {

return openLink(connection, "sender_open", "sender_error", connection.open_sender.bind(connection), {

snd_settle_mode: 1,

rcv_settle_mode: 0,

name: "management-link-pair",

target: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false },

source: { address: "/management", expiry_policy: "LINK_DETACH", timeout: 0, dynamic: false, durable: 0 },

properties: { paired: true },

})

}

async function openLink(

connection: Connection,

successEvent: string,

errorEvent: string,

openMethod: (config: any) => void,

config: any

): Promise<any> {

return new Promise((res, rej) => {

connection.once(successEvent, (context) => {

return res(context.receiver || context.sender)

})

connection.once(errorEvent, (context) => {

return rej(context.connection.error)

})

openMethod(config)

…env var

apietroni51 and others added 4 commits May 21, 2025 15:20

[wip] environment create connection test

865017e

feat: add rhea exploration test for connection creation

9550987

chore: revert tests

6e67790

feat: add environment creation and connection handling

332fb3d

l4mby requested a review from apietroni51 May 27, 2025 10:13

l4mby self-assigned this May 27, 2025

l4mby linked an issue May 27, 2025 that may be closed by this pull request

As a library user I want to create the Environment in order to allocate resources for the connections #9

Closed

l4mby requested review from gpad and removed request for apietroni51 May 27, 2025 10:14

chore: fix workflow rabbit image

a861fbd

gpad requested review from Copilot and removed request for gpad May 27, 2025 13:10

Copilot AI reviewed May 27, 2025

View reviewed changes

gpad reviewed May 27, 2025

View reviewed changes

magne added 2 commits May 28, 2025 08:57

chore: fix pr comments and add errors in promise rejection

bd63777

chore: remove unnecessary wait in test

6dd56ae

gpad requested a review from Copilot May 28, 2025 07:16

Copilot AI reviewed May 28, 2025

View reviewed changes

magne added 2 commits May 28, 2025 11:03

chore: refactor rhea tests

e20dab7

chore: enhance error in eventually function and remove unused docker …

b35b006

…env var

l4mby requested review from albertobarrila and apietroni51 and removed request for albertobarrila and apietroni51 June 4, 2025 14:37

l4mby merged commit 6f0a4eb into main Jun 5, 2025
5 checks passed

albertobarrila deleted the 9-as-a-library-user-i-want-to-create-the-environment-in-order-to-allocate-resources-for-the-connections branch July 4, 2025 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IS-9/feat]: as a library user i want to create the environment in order to allocate resources for the connections #12

[IS-9/feat]: as a library user i want to create the environment in order to allocate resources for the connections #12

Uh oh!

l4mby commented May 27, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 27, 2025

Uh oh!

Copilot AI May 27, 2025

Uh oh!

Copilot AI May 27, 2025

Uh oh!

gpad left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 28, 2025

Uh oh!

Copilot AI May 28, 2025

Uh oh!

Copilot AI May 28, 2025

Uh oh!

Copilot AI May 28, 2025

Uh oh!

Copilot AI May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	expect.fail(error as string)
	expect.fail(error instanceof Error ? error.message : String(error))

	rej()
	rej(context.error \|\| new Error("Connection error occurred"))

	return rej(false)
	return rej(new Error("Connection error: " + context.connection.error))

	describe.skip("Management", () => {
	describe("Management", () => {

[IS-9/feat]: as a library user i want to create the environment in order to allocate resources for the connections #12

[IS-9/feat]: as a library user i want to create the environment in order to allocate resources for the connections #12

Uh oh!

Conversation

l4mby commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

gpad left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

l4mby commented May 27, 2025 •

edited

Loading