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

[EventEngine] Refactoring the EventEngine Test Suite: Part 1 #32127

Merged
merged 22 commits into from Jan 24, 2023

Conversation

drfloob
Copy link
Member

@drfloob drfloob commented Jan 18, 2023

This is a partial refactoring, shortened to support @ctiller's work improving fix_build_deps.

Known limitations:

  • redundancy in test suite configuration
  • temporary locations for some EventEngine test utils should likely be moved into the src tree
  • A few new TODOs, to be cleaned up in Part 2+

@drfloob drfloob added the release notes: yes Indicates if PR needs to be in release notes label Jan 18, 2023
@ctiller ctiller mentioned this pull request Jan 18, 2023
@drfloob drfloob marked this pull request as ready for review January 18, 2023 20:33
Copy link
Contributor

@Vignesh2208 Vignesh2208 left a comment

Choose a reason for hiding this comment

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

Mostly lgtm. Left a few suggestions

@@ -241,5 +217,29 @@ ConnectionManager::CreateConnection(std::string target_addr,
return absl::CancelledError("Failed to create connection.");
}

// Returns a random message with bounded length.
std::string GetNextSendMessage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why this function was moved to the end of this file from the beginning of the file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it was a merge artifact, we had both done similar changes here. I've moved it back up near the top.

@@ -132,7 +131,38 @@ class ConnectionManager {
std::unique_ptr<EventEngine> oracle_event_engine_;
};

void AppendStringToSliceBuffer(SliceBuffer* buf, absl::string_view data);

std::string ExtractSliceBufferIntoString(SliceBuffer* buf);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ExtractSliceBufferIntoString is declared twice: here and in line 136.

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 catch, fixed

@@ -1,5 +1,7 @@
A reusable test suite for EventEngine implementations.

# Customizing tests for your EventEngine implementation

To exercise a custom EventEngine, create a new bazel test target that links
against the `//test/core/event_engine/test_suite:complete` library, and provide
Copy link
Contributor

Choose a reason for hiding this comment

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

//test/core/event_engine/test_suite:complete appears to be deleted. Does this line still hold true ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Documentation updated, thanks.

namespace grpc_event_engine {
namespace experimental {

void InitClientTests() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is expected to go in this function implementation ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing, unfortunately. This bit of code allows fix_build_deps to see a source file dependency between the custom test specifications and the test suite test implementations. They were previously only associated at link-time, which is fundamentally opposed to what fix_build_deps is trying to do. Without this hack, the tests themselves get removed from the test suite targets.

@drfloob drfloob merged commit 3980ed7 into grpc:master Jan 24, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jan 24, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
)

* Add back EventEngine test suite tools (echo binary)

This reverts commit d5c1219.

* more refactoring

* fix

* Automated change: Fix sanity tests

* fix

* fix posix oracle build def

* fix FuzzingEventEngine includes

* sanitize

* tidy and rm DNS

* fix cfstream includes

* header guards

* format & fix deps

* fix

* fix

* iwyu

* fix

* remove redundant code

* reviewer feedback

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
* Add back EventEngine test suite tools (echo binary)

This reverts commit d5c1219.

* more refactoring

* fix

* Automated change: Fix sanity tests

* fix

* fix posix oracle build def

* fix FuzzingEventEngine includes

* sanitize

* tidy and rm DNS

* fix cfstream includes

* header guards

* format & fix deps

* fix

* fix

* iwyu

* fix

* remove redundant code

* reviewer feedback

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants