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

[Service Bus] Remove test reference in src and fix test failures #8664

Merged
merged 3 commits into from
May 2, 2020
Merged

[Service Bus] Remove test reference in src and fix test failures #8664

merged 3 commits into from
May 2, 2020

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented May 1, 2020 •
edited
Loading

  • Removing the unintentional uncaught test reference in the src code
  • Updating the tests to fix test failures
    • Making the supposed MessagingError an actual MessagingError

@HarshaNalluru HarshaNalluru changed the title [Service Bus] translatedError instanceof MessagingError [Service Bus] Remove test reference in src May 1, 2020
chradek
chradek approved these changes May 1, 2020
View reviewed changes
Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Normally instanceof checks scare me since it would fail if the instance of that class you had came from a different copy of the library it's exported from...but in this case it's done internally and we know MessagingError is coming from core-amqp.
:shipit:

richardpark-msft
richardpark-msft approved these changes May 1, 2020
View reviewed changes
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks okay. I do wish we could just establish that the value of the error is undefined and just not keep handling it (there are two spots in there where we properly handle it but it's not obvious).

ramya-rao-a
ramya-rao-a reviewed May 1, 2020
View reviewed changes
sdk/servicebus/service-bus/test/streamingReceiver.spec.ts
@@ -1214,7 +1214,7 @@ describe("Streaming - onDetached", function(): void {
await receiverIsActive;

// Simulate onDetached being called with a non-retryable error.
const nonRetryableError = translate(new Error(`I break systems.`));
const nonRetryableError = new MessagingError(`I break systems.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the MessagingError class, retryable is set to true by default. So why this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

The next line sets retryable to false :)

Copy link
Member Author

@HarshaNalluru HarshaNalluru May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In the very next line, we set the retryable flag to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

And previously, translate wasn't converting this to a MessagingError (it did in amqp-common)

Copy link
Member Author

@HarshaNalluru HarshaNalluru May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes to that.
Since the last commit to that PR changed the logic of handling the non-messaging-errors, these tests failed.
If I had executed these tests in that PR before merging, would've captured the issue much before.

@HarshaNalluru
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HarshaNalluru
Copy link
Member Author

Looks okay. I do wish we could just establish that the value of the error is undefined and just not keep handling it

undefined instanceOf MessagingError would result in false. So, it's fine.

(there are two spots in there where we properly handle it but it's not obvious).

What two spots, @richardpark-msft ?

@HarshaNalluru HarshaNalluru changed the title [Service Bus] Remove test reference in src [Service Bus] Remove test reference in src and fix test failures May 2, 2020
@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented May 2, 2020 •
edited
Loading

The 4 "disconnect" tests failed in the browser
image

  • The uncaught TypeError is from rhea.
    image
    • This line is failing in the browser but not in the node.
    • _connection.idle() from the test triggers a settimeout, which invokes the abort_socket function.
    • When the test hits the highlighted line, the error is thrown.
      Here, this.socket.removeAllListeners is a function in node, but is undefined in browser.
    • Looks like rhea should be handling this in a better way since it is a valid code flow in the browser.
      (Chris can confirm!)
  • I was able to repro the issue in the browser even in track 1 (Since it didn't have #RunInBrowser tag, the test did not execute in track 1).
  • Skipping these tests in the browser until we get the issue fixed as Ramya suggested 371dc09

/cc - @chradek

@HarshaNalluru HarshaNalluru mentioned this pull request May 2, 2020
Closed
@HarshaNalluru HarshaNalluru merged commit 893bfc9 into Azure:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardpark-msft richardpark-msft Awaiting requested review from richardpark-msft

@chradek chradek Awaiting requested review from chradek

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HarshaNalluru @ramya-rao-a @chradek @richardpark-msft