-
Notifications
You must be signed in to change notification settings - Fork 21
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
[FLINK-35548] Add E2E tests for PubSubSinkV2 #28
Conversation
@snuyanzin Could you please have a look at this much smaller PR? |
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...ub-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/util/PubsubHelper.java
Outdated
Show resolved
Hide resolved
9a374f2
to
1e45ee3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vahmed-hamdy thanks for driving this PR.
I left a few comments, PTAL. Also,I think we need IT tests for failing scenarios (e.g. for when error shutting down topic client, admin client, channel, etc). WDYT?
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Show resolved
Hide resolved
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...ub-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/util/PubsubHelper.java
Show resolved
Hide resolved
...ub-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/util/PubsubHelper.java
Show resolved
Hide resolved
...ub-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/util/PubsubHelper.java
Outdated
Show resolved
Hide resolved
1e45ee3
to
f96ed11
Compare
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...-e2e-tests/src/test/java/org/apache/flink/connector/gcp/pubsub/sink/PubSubSinkV2ITTests.java
Outdated
Show resolved
Hide resolved
...a/org/apache/flink/streaming/connectors/gcp/pubsub/emulator/EmulatorCredentialsProvider.java
Show resolved
Hide resolved
f96ed11
to
dbe1cdd
Compare
@jeyhunkarimov thanks for the feedback, I added a negative test as well |
Thanks for addressing feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vahmed-hamdy LGTM!
dbe1cdd
to
5490616
Compare
@snuyanzin All addressed now ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing feedback and thanks for contribution @vahmed-hamdy
thanks for the review @jeyhunkarimov , @morazow
LGTM
Description
Add E2E tests for
PubsubSinkV2
Verification