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.

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

Improve resiliance of network request registration and error messages #635

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

lyonsil
Copy link
Member

@lyonsil lyonsil commented Nov 9, 2023

I'm applying some changes make to the tech demo branch back to main.

The most important change here is in PapiClient.cs. If the network service doesn't respond to a request registration within the expected time, then we were causing an exception that hid what was going on. I also increased the timeout (which we could decide to ditch entirely later if we want).

I changed several response error messages to include call stacks because just giving the exception message wasn't enough to easily track down the problem.


This change is Reviewable

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Looks fine to me however it looks like you need to fix some tests.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Indeed - Tim's tests were put in after I pulled the tech demo branch, so I didn't see the failures there.

Reviewable status: 5 of 6 files reviewed, all discussions resolved

@lyonsil lyonsil enabled auto-merge (squash) November 9, 2023 20:27
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)


c-sharp-tests/PapiTestBase.cs line 221 at r2 (raw file):

                MessageResponse response = (MessageResponse)message;
                Assert.That(response.ErrorMessage ?? "", Does.Contain(expectedErrorMessage ?? ""));

What if response.ErrorMessage is null but expectedErrorMessage is not? This will pass won't it?

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)


c-sharp-tests/PapiTestBase.cs line 221 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

What if response.ErrorMessage is null but expectedErrorMessage is not? This will pass won't it?

That would not pass. If response.ErrorMessage is null and the expected is not, then it would be checking if the empty string contains something that isn't empty.

I think you might concerned about the opposite. That is, if the actual error message is not null and the expected is null, then this line would pass. However, in that case the next line would fail. That is, the actual response would be false for success, but the expected case would be true.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lyonsil lyonsil merged commit d125a12 into main Nov 9, 2023
7 checks passed
@lyonsil lyonsil deleted the handle-slow-network-service branch November 9, 2023 22:34
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