-
Notifications
You must be signed in to change notification settings - Fork 17
chore(E2E): add dedicated topic fixture for creation/deletion
#3020
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
Conversation
73f9cf0 to
147ada5
Compare
| /** | ||
| * Set up a topic based on the {@linkcode topicConfig} option and return the associated topic | ||
| * `name` for tests to reference. | ||
| */ | ||
| topic: string; |
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.
I've debated making this return { name: string } but I don't know what else we would need to return in here in the future, and changing the fixture to topicName felt weird from the perspective of illustrating what the fixture does (setup- and teardown-wise)
77dc45d to
b343df2
Compare
cb7af7c to
8741390
Compare
0c800da to
8982f73
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
8982f73 to
5a4ad9a
Compare
760bb0f to
6c611c9
Compare
…e for tests to use
6c611c9 to
0791b1d
Compare
…traces for more CI debugging
| // we don't check for the "Local" item here in the event the Confluent Cloud item has children | ||
| // and is expanded, because it may push the Local item out of view and adding logic in here to | ||
| // scroll to it would be more complex than necessary for this utility function |
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.
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
| retries: 2, | ||
| timeout: 120000, | ||
| workers: 1, | ||
| timeout: 120_000 * CI_FACTOR * WINDOWS_FACTOR, // 2min to 8min on CI Windows |
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.
We may be able to drop the base value for this more, but it should be fine here for now. CI+Windows added for consistency to match the expect.timeout config
Cerchie
left a comment
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.
LGTM! Ran the specs locally


Summary of Changes
We had a few test suites that implemented their own logic to make sure a Kafka topic was created before the actual-test logic ran (and similarly implemented their own logic to delete said topic afterward).
This PR adds a new
topicfixture to consolidate creation and deletion logic into one place. No functional changes, nor explicit behavioral changes to affected tests, but is required for #3021Closes #3014
Optional: Any additional details or context that should be provided?
@requires-topictag to make it easier to run these affected tests, and may extend the pattern in follow-up branches for things like@requires-schema. (I don't think we'll need@requires-kafka-cluster/@requires-schema-registry/@requires-flink-compute-pollanytime soon, but can revisit later.)use.viewPort.[height|width]properties don't work here, andpage.setViewportSize( ... )causes some very odd clipping issues where VS Code UI elements are out of the main playwright viewing area. It's probably better for these tests (and the extension) to be able to work with differently-sized windows anyway, so I'm not pursuing this viewport effort further for now.Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Release notes