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

Added unit tests for remote weavelet. #528

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Conversation

mwhittaker
Copy link
Member

This PR adds unit tests for the remote weavelet. Specifically, it tests that the weavelet still operates correctly even when some of its interactions with the envelope fail. For example, the weavelet should retry failed ActivateComponent calls. A handful of the tests are currently failing because of "bugs" in the remote weavelet. I skipped the tests for now and will try to fix the bugs in future PRs.

Fixing the bugs is a bit tricky because it's not always straightforward the best way to handle failures. For example, if a weavelet calls GetListenerAddress and receives an address that is already in use, should it keep retrying the GetListenerAddress? Or should it just self-terminate?

@mwhittaker mwhittaker requested a review from spetrovic77 August 15, 2023 21:55
@mwhittaker mwhittaker self-assigned this Aug 15, 2023
Copy link
Contributor

@spetrovic77 spetrovic77 left a comment

Choose a reason for hiding this comment

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

Awesome testing!!

d.t.Fatalf("A(%d): got %d, want %d", want, got, want)
}

// Test component b.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding: are these tests necessary, given that there's a call chain A -> B -> C ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I don't think they're necessary. I removed them.

This PR adds unit tests for the remote weavelet. Specifically, it tests
that the weavelet still operates correctly even when some of its
interactions with the envelope fail. For example, the weavelet should
retry failed ActivateComponent calls. A handful of the tests are
currently failing because of "bugs" in the remote weavelet. I skipped
the tests for now and will try to fix the bugs in future PRs.

Fixing the bugs is a bit tricky because it's not always straightforward
the best way to handle failures. For example, if a weavelet calls
GetListenerAddress and receives an address that is already in use,
should it keep retrying the GetListenerAddress? Or should it just
self-terminate?
- Removed redundant component tests.
- Added nolint directive to make linter happy.
- Added TestLogger to avoid logging after a test ends.
@mwhittaker mwhittaker force-pushed the remoteweavelet_tests branch from 8c399c9 to cc31f73 Compare August 21, 2023 16:59
@mwhittaker mwhittaker merged commit 9c40463 into main Aug 21, 2023
@mwhittaker mwhittaker deleted the remoteweavelet_tests branch August 21, 2023 17:05
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.

2 participants