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

E2E upload object test #8631

Merged
merged 12 commits into from
Feb 11, 2025
Merged

E2E upload object test #8631

merged 12 commits into from
Feb 11, 2025

Conversation

ItamarYuran
Copy link
Contributor

@ItamarYuran ItamarYuran commented Feb 9, 2025

Closes #8542

@ItamarYuran ItamarYuran added the exclude-changelog PR description should not be included in next release changelog label Feb 9, 2025
@ItamarYuran ItamarYuran marked this pull request as draft February 9, 2025 13:10
@ItamarYuran ItamarYuran changed the title Removed AWS secrets E2E upload object test Feb 9, 2025
Copy link

github-actions bot commented Feb 9, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

Copy link

github-actions bot commented Feb 9, 2025

E2E Test Results - Quickstart

11 passed

@ItamarYuran ItamarYuran requested a review from a team February 9, 2025 18:25
@ItamarYuran ItamarYuran marked this pull request as ready for review February 9, 2025 18:25
Copy link
Contributor

@idanovo idanovo left a comment

Choose a reason for hiding this comment

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

Looks good!
Some minor (I hope) comments

Comment on lines 13 to 17
test("Create repo", async ({page}) => {
const repositoriesPage = new RepositoriesPage(page);
await repositoriesPage.goto();
await repositoriesPage.createRepository(TEST_REPO_NAME, true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

The create repository should be a step in the same test, the upload file test shouldn't test the create repository step


const repositoryPage = new RepositoryPage(page);
await repositoryPage.uploadObject(filePath);
await expect(page.getByRole('complementary')).toContainText(`lakefs://${TEST_REPO_NAME}/main/${FILE_NAME}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a check that we can see the new file at the right path.

Comment on lines 128 to 132
async uploadObject(filePath: string): Promise<void> {
await this.page.getByRole("button", { name: "Upload Object" }).click();
await this.page.getByText("Drag 'n' drop files or").click();
const fileInput = await this.page.locator('input[type="file"]');
await fileInput.setInputFiles(filePath);
Copy link
Contributor

@idanovo idanovo Feb 10, 2025

Choose a reason for hiding this comment

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

Suggested change
async uploadObject(filePath: string): Promise<void> {
await this.page.getByRole("button", { name: "Upload Object" }).click();
await this.page.getByText("Drag 'n' drop files or").click();
const fileInput = await this.page.locator('input[type="file"]');
await fileInput.setInputFiles(filePath);
async uploadObject(filePath: string): Promise<void> {
await this.page.getByRole("button", { name: "Upload Object" }).click();
await this.page.getByText("Drag 'n' drop files or").click();
const fileInput = await this.page.locator('input[type="file"]');
await fileInput.setInputFiles(filePath);

@ItamarYuran ItamarYuran requested a review from idanovo February 10, 2025 14:06
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

Nice job!

If you want to verify this indeed catches the bug we had you can try running this test scenario on v1.48.2 and see if it fails

Copy link
Contributor

@idanovo idanovo left a comment

Choose a reason for hiding this comment

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

LGTM!

@ItamarYuran ItamarYuran merged commit 7f58940 into master Feb 11, 2025
39 checks passed
@ItamarYuran ItamarYuran deleted the e2e-upload branch February 11, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add E2E upload object test
3 participants