-
Notifications
You must be signed in to change notification settings - Fork 17
Testing too large file dimension #3182
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
Changes from all commits
04ba678
7ae2b93
9e39402
a95e196
35c4e3c
000f695
ab1629b
a4740a8
4132cac
29a2eff
db00d4b
bfe87f8
297948b
2d26ddb
299e062
30f967a
fb8bf64
1f6942a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,20 +131,24 @@ export class FlinkDatabaseView extends SearchableView { | |
| * @param electronApp - The Electron application instance | ||
| * @param filePath - The path to the JAR file to upload | ||
| * @param skipInitiation - If true, skips clicking the upload button (assumes quickpick is already open) | ||
| * @param expectSuccess - If true, waits for success notification; if false, caller handles error notification | ||
| * @returns The name of the uploaded artifact | ||
| */ | ||
| async uploadFlinkArtifact( | ||
| electronApp: ElectronApplication, | ||
| filePath: string, | ||
| skipInitiation = false, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| if (!skipInitiation) { | ||
| await this.initiateUpload(); | ||
| } | ||
| await this.selectJarFile(electronApp, filePath); | ||
| const artifactName = await this.enterArtifactName(filePath); | ||
| await this.confirmUpload(); | ||
| await this.waitForUploadSuccess(); | ||
| if (expectSuccess) { | ||
| await this.waitForUploadSuccess(); | ||
| } | ||
| return artifactName; | ||
| } | ||
|
|
||
|
|
@@ -306,9 +310,14 @@ export class FlinkDatabaseView extends SearchableView { | |
| * Navigates through the quickpick steps after the upload has been initiated from a JAR file. | ||
| * @param artifactName - The name of the uploaded artifact (for verification) | ||
| * @param providerRegion - Optional provider/region to match (e.g., "AWS/us-east-2") | ||
| * @param expectSuccess - If true, waits for success notification | ||
| * @returns The name of the uploaded artifact | ||
| */ | ||
| async uploadFlinkArtifactFromJAR(artifactName: string, providerRegion?: string): Promise<string> { | ||
| async uploadFlinkArtifactFromJAR( | ||
| artifactName: string, | ||
| providerRegion?: string, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| // Wait for the quickpick to appear | ||
| const quickpick = new Quickpick(this.page); | ||
| await expect(quickpick.locator).toBeVisible(); | ||
|
|
@@ -357,12 +366,14 @@ export class FlinkDatabaseView extends SearchableView { | |
| await expect(uploadAction).toBeVisible(); | ||
| await uploadAction.click(); | ||
|
|
||
| // Wait for upload success notification | ||
| const notificationArea = new NotificationArea(this.page); | ||
| const successNotifications = notificationArea.infoNotifications.filter({ | ||
| hasText: "uploaded successfully", | ||
| }); | ||
| await expect(successNotifications.first()).toBeVisible(); | ||
| if (expectSuccess) { | ||
| // Wait for upload success notification | ||
| const notificationArea = new NotificationArea(this.page); | ||
| const successNotifications = notificationArea.infoNotifications.filter({ | ||
| hasText: "uploaded successfully", | ||
| }); | ||
| await expect(successNotifications.first()).toBeVisible(); | ||
| } | ||
|
Comment on lines
+369
to
+376
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't have to be in this branch, but I think we should move these out into the individual tests themselves so too much test-logic isn't inside of these page object models. |
||
|
|
||
| return artifactName; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,18 @@ | ||
| import type { ElectronApplication, Page } from "@playwright/test"; | ||
| import type { ElectronApplication, Locator, Page } from "@playwright/test"; | ||
| import { expect } from "@playwright/test"; | ||
| import { stubDialog } from "electron-playwright-helpers"; | ||
| import * as path from "path"; | ||
| import { fileURLToPath } from "url"; | ||
| import { test } from "../baseTest"; | ||
| import { ConnectionType } from "../connectionTypes"; | ||
| import { FileExplorer } from "../objects/FileExplorer"; | ||
| import { NotificationArea } from "../objects/notifications/NotificationArea"; | ||
| import { Quickpick } from "../objects/quickInputs/Quickpick"; | ||
| import { FlinkDatabaseView, SelectFlinkDatabase } from "../objects/views/FlinkDatabaseView"; | ||
| import { ViewItem } from "../objects/views/viewItems/ViewItem"; | ||
| import { Tag } from "../tags"; | ||
| import { executeVSCodeCommand } from "../utils/commands"; | ||
| import { cleanupLargeFile, createLargeFile } from "../utils/flinkDatabase"; | ||
| import { openConfluentSidebar } from "../utils/sidebarNavigation"; | ||
| import { randomHexString } from "../utils/strings"; | ||
|
|
||
|
|
@@ -31,22 +33,33 @@ test.describe("Flink Artifacts", { tag: [Tag.CCloud, Tag.FlinkArtifacts] }, () = | |
| "udfs-simple.jar", | ||
| ); | ||
|
|
||
| const fixturesDir = path.join(__dirname, "..", "..", "fixtures", "flink-artifacts"); | ||
Cerchie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const invalidFiles = [ | ||
| { | ||
| description: "oversized artifact (>100MB)", | ||
| setupFile: () => createLargeFile({ sizeInMB: 150, directory: fixturesDir }), | ||
| cleanupFile: (filePath: string) => cleanupLargeFile(filePath), | ||
| shouldSucceed: false, | ||
| }, | ||
| ]; | ||
|
|
||
| const entrypoints = [ | ||
| { | ||
| entrypoint: SelectFlinkDatabase.FromDatabaseViewButton, | ||
| testName: "should upload Flink Artifact when cluster selected from Artifacts view button", | ||
| testName: "cluster selected from Artifacts view button", | ||
| }, | ||
| { | ||
| entrypoint: SelectFlinkDatabase.DatabaseFromResourcesView, | ||
| testName: "should upload Flink Artifact when cluster selected from the Resources view", | ||
| testName: "cluster selected from the Resources view", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not successful every time now |
||
| }, | ||
| { | ||
| entrypoint: SelectFlinkDatabase.ComputePoolFromResourcesView, | ||
| testName: "should upload Flink Artifact when cluster selected from Flink Compute Pool", | ||
| testName: "cluster selected from Flink Compute Pool", | ||
| }, | ||
| { | ||
| entrypoint: SelectFlinkDatabase.JarFile, | ||
| testName: "should upload Flink Artifact when initiated from JAR file in file explorer", | ||
| testName: "initiated from JAR file in file explorer", | ||
| }, | ||
| ]; | ||
|
|
||
|
|
@@ -56,37 +69,73 @@ test.describe("Flink Artifacts", { tag: [Tag.CCloud, Tag.FlinkArtifacts] }, () = | |
| { provider: "AZURE", region: "eastus" }, | ||
| ]; | ||
|
|
||
| for (const config of entrypoints) { | ||
| for (const providerRegion of providersWithRegions) { | ||
| test.describe(`with ${providerRegion.provider}/${providerRegion.region}`, () => { | ||
| const { provider, region } = providerRegion; | ||
| test(config.testName, async ({ page, electronApp }) => { | ||
| await setupTestEnvironment(config.entrypoint, page, electronApp); | ||
| const artifactsView = new FlinkDatabaseView(page); | ||
| for (const { entrypoint, testName } of entrypoints) { | ||
| for (const { provider, region } of providersWithRegions) { | ||
| test(`should upload a jar and create an artifact successfully [${provider}/${region}] - ${testName}`, async ({ | ||
| page, | ||
| electronApp, | ||
| }) => { | ||
| await setupTestEnvironment(entrypoint, page, electronApp); | ||
| const artifactsView = new FlinkDatabaseView(page); | ||
|
|
||
| await artifactsView.ensureExpanded(); | ||
| await artifactsView.loadArtifacts(config.entrypoint); | ||
| await artifactsView.ensureExpanded(); | ||
| await artifactsView.loadArtifacts(entrypoint); | ||
| const uploadedArtifactName = await startUploadFlow( | ||
| entrypoint, | ||
| page, | ||
| electronApp, | ||
| artifactsView, | ||
| provider, | ||
| region, | ||
| artifactPath, | ||
| ); | ||
|
|
||
| const uploadedArtifactName = await startUploadFlow( | ||
| config.entrypoint, | ||
| const artifactViewItem = await artifactsView.getDatabaseResourceByLabel( | ||
| uploadedArtifactName, | ||
| artifactsView.artifactsContainer, | ||
| ); | ||
|
|
||
| await expect(artifactViewItem).toBeVisible(); | ||
| await artifactsView.deleteFlinkArtifact(uploadedArtifactName); | ||
| await expect(artifactsView.artifacts.filter({ hasText: uploadedArtifactName })).toHaveCount( | ||
| 0, | ||
| ); | ||
| }); | ||
|
|
||
| test(`should fail to upload a jar exceeding the file limit [${provider}/${region}] - ${testName}`, async ({ | ||
| page, | ||
| electronApp, | ||
| }) => { | ||
| await setupTestEnvironment(entrypoint, page, electronApp); | ||
| const artifactsView = new FlinkDatabaseView(page); | ||
|
|
||
| await artifactsView.ensureExpanded(); | ||
| await artifactsView.loadArtifacts(entrypoint); | ||
| const initialArtifactCount = await artifactsView.artifacts.count(); | ||
| const pathToBigArtifact = createLargeFile({ sizeInMB: 150, directory: fixturesDir }); | ||
| try { | ||
| await startUploadFlow( | ||
| entrypoint, | ||
| page, | ||
| electronApp, | ||
| artifactsView, | ||
| provider, | ||
| region, | ||
| pathToBigArtifact, | ||
| false, // expectSuccess - we expect this upload to fail | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a little odd - what errors do we expect here? It may be clearer to expect the error notification instead of handling the failed info-notification assertion |
||
| ); | ||
| } catch (error) { | ||
| // Swallow any errors from the upload flow since we expect failure | ||
| } | ||
|
|
||
| const artifactViewItem = await artifactsView.getDatabaseResourceByLabel( | ||
| uploadedArtifactName, | ||
| artifactsView.artifactsContainer, | ||
| ); | ||
| await expect(artifactsView.artifacts).toHaveCount(initialArtifactCount); | ||
|
|
||
| await expect(artifactViewItem).toBeVisible(); | ||
| await artifactsView.deleteFlinkArtifact(uploadedArtifactName); | ||
| await expect( | ||
| artifactsView.artifacts.filter({ hasText: uploadedArtifactName }), | ||
| ).toHaveCount(0); | ||
| const notificationArea = new NotificationArea(page); | ||
| const failureNotifications: Locator = notificationArea.errorNotifications.filter({ | ||
| hasText: /Failed to upload/, | ||
Cerchie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
| await expect(failureNotifications.first()).toBeVisible(); | ||
| cleanupLargeFile(pathToBigArtifact); | ||
Cerchie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
| } | ||
| } | ||
|
|
@@ -98,8 +147,6 @@ test.describe("Flink Artifacts", { tag: [Tag.CCloud, Tag.FlinkArtifacts] }, () = | |
| ): Promise<void> { | ||
| // JAR file test requires opening the fixtures folder as a workspace | ||
| if (entrypoint === SelectFlinkDatabase.JarFile) { | ||
| const fixturesDir = path.join(__dirname, "..", "..", "fixtures", "flink-artifacts"); | ||
|
|
||
| await stubDialog(electronApp, "showOpenDialog", { | ||
| filePaths: [fixturesDir], | ||
| }); | ||
|
|
@@ -121,21 +168,41 @@ test.describe("Flink Artifacts", { tag: [Tag.CCloud, Tag.FlinkArtifacts] }, () = | |
| artifactsView: FlinkDatabaseView, | ||
| provider: string, | ||
| region: string, | ||
| filePath: string, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| switch (entrypoint) { | ||
| case SelectFlinkDatabase.DatabaseFromResourcesView: | ||
| return await completeArtifactUploadFlow(electronApp, artifactPath, artifactsView); | ||
| return await completeArtifactUploadFlow( | ||
| electronApp, | ||
| filePath, | ||
| artifactsView, | ||
| expectSuccess, | ||
| ); | ||
| case SelectFlinkDatabase.FromDatabaseViewButton: | ||
| return await completeArtifactUploadFlow(electronApp, artifactPath, artifactsView); | ||
| return await completeArtifactUploadFlow( | ||
| electronApp, | ||
| filePath, | ||
| artifactsView, | ||
| expectSuccess, | ||
| ); | ||
| case SelectFlinkDatabase.ComputePoolFromResourcesView: | ||
| return await completeUploadFlowForComputePool(electronApp, artifactsView, provider, region); | ||
| return await completeUploadFlowForComputePool( | ||
| electronApp, | ||
| artifactsView, | ||
| provider, | ||
| region, | ||
| filePath, | ||
| expectSuccess, | ||
| ); | ||
| case SelectFlinkDatabase.JarFile: | ||
| return await completeArtifactUploadFlowForJAR( | ||
| page, | ||
| artifactPath, | ||
| filePath, | ||
| artifactsView, | ||
| provider, | ||
| region, | ||
| expectSuccess, | ||
| ); | ||
| } | ||
| } | ||
|
|
@@ -145,13 +212,16 @@ test.describe("Flink Artifacts", { tag: [Tag.CCloud, Tag.FlinkArtifacts] }, () = | |
| artifactsView: FlinkDatabaseView, | ||
| provider: string, | ||
| region: string, | ||
| filePath: string, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| await artifactsView.clickUploadFromComputePool(provider, region); | ||
| // Skip initiation since the upload modal was already opened via the compute pool context menu | ||
| const uploadedArtifactName = await artifactsView.uploadFlinkArtifact( | ||
| electronApp, | ||
| artifactPath, | ||
| filePath, | ||
| true, | ||
| expectSuccess, | ||
| ); | ||
|
|
||
| await artifactsView.selectKafkaClusterByProviderRegion(provider, region); | ||
|
|
@@ -165,9 +235,9 @@ async function completeArtifactUploadFlow( | |
| electronApp: ElectronApplication, | ||
| artifactPath: string, | ||
| artifactsView: FlinkDatabaseView, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| const uploadedArtifactName = await artifactsView.uploadFlinkArtifact(electronApp, artifactPath); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. felt redundant |
||
| return uploadedArtifactName; | ||
| return await artifactsView.uploadFlinkArtifact(electronApp, artifactPath, false, expectSuccess); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -180,6 +250,7 @@ async function completeArtifactUploadFlowForJAR( | |
| artifactsView: FlinkDatabaseView, | ||
| provider: string, | ||
| region: string, | ||
| expectSuccess = true, | ||
| ): Promise<string> { | ||
| // Use the artifact file name (without extension) as the artifact name | ||
| const baseFileName = path.basename(artifactPath, ".jar"); | ||
|
|
@@ -194,7 +265,11 @@ async function completeArtifactUploadFlowForJAR( | |
| const fileItem = new ViewItem(page, jarFile); | ||
| await fileItem.rightClickContextMenuAction("Upload Flink Artifact to Confluent Cloud"); | ||
|
|
||
| await artifactsView.uploadFlinkArtifactFromJAR(artifactName, `${provider}/${region}`); | ||
| await artifactsView.uploadFlinkArtifactFromJAR( | ||
| artifactName, | ||
| `${provider}/${region}`, | ||
| expectSuccess, | ||
| ); | ||
|
|
||
| // Switch back to the Confluent extension sidebar from the file explorer | ||
| await openConfluentSidebar(page); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||
| import * as fs from "fs"; | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new file, will also set the stage for adding a util that create an invalid JAR file for the next dimension |
||||||
| import * as os from "os"; | ||||||
| import * as path from "path"; | ||||||
|
|
||||||
| export interface LargeFileOptions { | ||||||
| /** Size in megabytes. Default is 101MB (just over the 100MB threshold) */ | ||||||
| sizeInMB?: number; | ||||||
| /** Custom filename. If not provided, a default name will be used */ | ||||||
| filename?: string; | ||||||
| /** Directory to create the file in. Defaults to system temp directory */ | ||||||
| directory?: string; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Creates a large file for testing artifact upload rejection. | ||||||
| * The file is filled with random data to simulate a real artifact. | ||||||
| * | ||||||
| * @param options - Configuration options for the large file | ||||||
| * @returns The absolute path to the created file | ||||||
| */ | ||||||
| export function createLargeFile(options: LargeFileOptions = {}): string { | ||||||
| const { | ||||||
| sizeInMB = 101, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: it may be clearer to set the size limit the extension uses as a top-level constant here, then just increment by 1 to indicate exceeding the size
Suggested change
|
||||||
| filename = `test-large-artifact-${Date.now()}.jar`, | ||||||
| directory = os.tmpdir(), | ||||||
| } = options; | ||||||
|
|
||||||
| const filePath = path.join(directory, filename); | ||||||
| const sizeInBytes = sizeInMB * 1024 * 1024; | ||||||
| const chunkSize = 1024 * 1024; // 1MB chunks for efficient writing | ||||||
|
|
||||||
| const buffer = Buffer.alloc(chunkSize, 0); // Fill with zeros (or any byte) | ||||||
|
|
||||||
| const fd = fs.openSync(filePath, "w"); | ||||||
| try { | ||||||
| for (let written = 0; written < sizeInBytes; written += chunkSize) { | ||||||
| const bytesToWrite = Math.min(chunkSize, sizeInBytes - written); | ||||||
| fs.writeSync(fd, buffer, 0, bytesToWrite); | ||||||
| } | ||||||
| } finally { | ||||||
| fs.closeSync(fd); // Always close even if error occurs | ||||||
| } | ||||||
| return filePath; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Cleans up (deletes) a large test file. | ||||||
| * @param filePath - The absolute path to the file to delete | ||||||
| */ | ||||||
| export function cleanupLargeFile(filePath: string): void { | ||||||
| if (fs.existsSync(filePath)) { | ||||||
| fs.unlinkSync(filePath); | ||||||
| } | ||||||
| } | ||||||
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.
added since we now expect failures sometimes