Skip to content

feat: Singleton PubSub Client#427

Open
AdityaKasar wants to merge 7 commits intodevfrom
feature/singleton-pubsub
Open

feat: Singleton PubSub Client#427
AdityaKasar wants to merge 7 commits intodevfrom
feature/singleton-pubsub

Conversation

@AdityaKasar
Copy link
Copy Markdown
Contributor

No description provided.

@AdityaKasar AdityaKasar force-pushed the feature/singleton-pubsub branch from d84c72d to 5dedc7f Compare May 20, 2025 09:21
Comment thread cypress/support/cypress-support/src/main.js Outdated
Comment thread cypress/support/cypress-support/src/main.js Outdated
} else {
cy.log(CONSTANTS.APP_TRANSPORT_UNAVAILABLE).then(() => {
fireLog.fail(CONSTANTS.APP_TRANSPORT_UNAVAILABLE);
assert(false, CONSTANTS.APP_TRANSPORT_UNAVAILABLE);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did we change from fireLog.fail to assert here?

Copy link
Copy Markdown
Contributor

@ksentak ksentak left a comment

Choose a reason for hiding this comment

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

Left a couple of comments for requested changes. Also, I want to have conversations about why there needed to be changes to how parsing was done for healthcheck and report. This alerts me that something might not be right. Perhaps what we incorrectly changed how an incoming message is parsed or something so we need to make sure that isn't the case.

@AdityaKasar
Copy link
Copy Markdown
Contributor Author

Also, I want to have conversations about why there needed to be changes to how parsing was done for healthcheck and report. This alerts me that something might not be right. Perhaps what we incorrectly changed how an incoming message is parsed or something so we need to make sure that isn't the case

Updated _handleIncomingMessage() logic in Websocket to handle response from (app) comcast.test.firecer differently.
if(data.topic.includes("comcast.test.firecert")){ callback(typeof payload === 'string' ? payload : JSON.stringify(payload), data.headers); }else{ callback(typeof data === 'string' ? data : JSON.stringify(data), data.headers); }

@AdityaKasar AdityaKasar requested a review from ksentak May 22, 2025 10:34
Copy link
Copy Markdown
Contributor

@ksentak ksentak left a comment

Choose a reason for hiding this comment

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

Minimal changes requested here. Otherwise looks good!


// Fetch the required appTransport from config module
appTransport = module.appTransport;
const firstExternalTransportKey = Object.keys(module.externalTransport)[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should work without issue. In the config helper we may want to make a comment to ensure the order of the exports stays the same. Otherwise this would break. Although I dont anticipate any other transportation methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added comment in transportLayers/index.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is problematic. It works for us but any future config modules would be blind to this issue.

This needs to be resolved before this is merged in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

made changes to address the review comment.

Comment thread cypress/support/cypress-support/src/main.js

// Fetch the required appTransport from config module
appTransport = module.appTransport;
const firstExternalTransportKey = Object.keys(module.externalTransport)[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is problematic. It works for us but any future config modules would be blind to this issue.

This needs to be resolved before this is merged in.

@github-actions
Copy link
Copy Markdown

Hello feature run failed with the following errors:


  1) An uncaught error was detected outside of a test:
     TypeError: The following error originated from your test code, not from Cypress.

  > Cannot read properties of undefined (reading 'default')

When Cypress detects uncaught errors originating from your test code it will automatically fail the current test.

Cypress could not associate this error to any specific test.

We dynamically generated a new test to display this failure.
      at main_default (http://localhost:45779/__cypress/tests?p=cypress/support/e2e.js:69732:48)
      at eval (http://localhost:45779/__cypress/tests?p=cypress/support/e2e.js:71475:3)
      at eval (http://localhost:45779/__cypress/tests?p=cypress/support/e2e.js:71486:3)
      at eval (<anonymous>)



For more information on our testing policies, please see our Testing-Guide.

@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 29, 2025
@AdityaKasar AdityaKasar requested review from kschrief and ksentak May 29, 2025 18:08
@rdkcentral rdkcentral deleted a comment from github-actions Bot May 30, 2025
ksentak
ksentak previously approved these changes May 30, 2025
Copy link
Copy Markdown
Contributor

@kschrief kschrief left a comment

Choose a reason for hiding this comment

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

I see no changes in the default config module. This must be taken into account before we can approve this.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2025

Issue with linting detected.
Linting failed with the following errors:

yarn run v1.22.22
$ eslint .

/home/runner/work/firebolt-certification-suite/firebolt-certification-suite/defaultModule/appTransport/index.js
  20:1  error  Delete `⏎`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

For more information on our linting policies, please see our Linting-Guide.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2025

Build error occurred while running sample test with the following errors:

Error: Build failed with 1 error:
node_modules/configModule/appTransport/index.js:19:25: ERROR: Could not resolve "fcs-config-helper/transportLayers/linchpin"
    at failureErrorWithLog (/home/runner/work/firebolt-certification-suite/firebolt-certification-suite/node_modules/esbuild/lib/main.js:1636:15)
    at /home/runner/work/firebolt-certification-suite/firebolt-certification-suite/node_modules/esbuild/lib/main.js:1048:25
    at runOnEndCallbacks (/home/runner/work/firebolt-certification-suite/firebolt-certification-suite/node_modules/esbuild/lib/main.js:1471:45)
    at buildResponseToResult (/home/runner/work/firebolt-certification-suite/firebolt-certification-suite/node_modules/esbuild/lib/main.js:1046:7)

For more information on our testing policies, please see our Testing-Guide.

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