-
Notifications
You must be signed in to change notification settings - Fork 45.9k
test(frontend): e2e tests for library page #10355
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
base: dev
Are you sure you want to change the base?
test(frontend): e2e tests for library page #10355
Conversation
## Changes 🏗️ ### The Issue - Backend returns: `"https://storage.googleapis.com/..."` (valid JSON string) - Frontend was calling `response.text()` which gave: `"\"https://storage.googleapis.com/...\""` - This resulted in a URL with extra quotes that couldn't be loaded ### The Fix I changed both file upload methods to use `response.json()` instead of `response.text()`: 1. **Client-side uploads** (`_makeClientFileUpload`): Changed `return await response.text();` to `return await response.json();` 2. **Server-side uploads** (`makeAuthenticatedFileUpload`): Changed `return await response.text();` to `return await response.json();` Now when the backend returns a JSON string like `"https://example.com/file.png"`, the frontend will properly parse it as JSON and extract just the URL without the quotes. ## Checklist 📋 ### For code changes: - [x] I have clearly listed my changes in the PR description - [x] I have made a test plan - [x] I have tested my changes according to the test plan: - [x] Login - [x] Upload an image on your profile - [x] It works ### For configuration changes: No configuration changes
This PR targets the Automatically setting the base branch to |
✅ Deploy Preview for auto-gpt-docs canceled.
|
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
✅ Deploy Preview for auto-gpt-docs canceled.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Here's the code health analysis summary for commits Analysis Summary
|
Thank you for adding these comprehensive e2e tests for the library page! This is valuable work that will help ensure the library functionality remains stable. However, there are a few issues that need to be addressed before this PR can be merged: Title FormatThe PR title has a typo ('frontent' should be 'frontend') and should use the singular 'test' instead of 'tests' to follow the conventional commit format. Please update to:
Missing ChecklistYour PR is missing the required checklist. Since you're adding substantial new test code, please include the complete checklist from the PR template, with appropriate items checked off. Potentially Out of Scope ChangesI noticed a couple of changes that appear unrelated to adding e2e tests:
Data-testid AdditionsYou've added several data-testid attributes to components. This is good for testing, but ensure these IDs follow any project conventions for test IDs. Once you've addressed these items, this PR will be ready for another review. The test implementation itself looks thorough and well-structured! |
Thanks for adding these comprehensive e2e tests for the library page! The tests look thorough and well-structured, covering many important aspects of the library functionality. A few observations:
Overall, this is a great addition that will improve test coverage of the library page. Once you've addressed the PR title and clarified the intended changes to the workflow, this should be ready to merge. |
Hi @Abhi1992002, thanks for adding these library page e2e tests! The tests themselves look comprehensive and well-structured. However, there are a few things that need to be addressed before this PR can be merged: PR Format Issues
Scope Issues
Next Steps
The test code itself looks good and comprehensive! Just need to address these documentation and scope issues before we can proceed with merging. |
Thank you for adding comprehensive e2e tests for the library page! The test coverage looks thorough with 9 tests covering various aspects of the library functionality. However, there are a few issues that need to be addressed before this PR can be merged:
The test implementation itself looks solid with good coverage of library functionality including search, pagination, sorting, and upload features. Please address the issues above, and this PR will be ready for another review. |
Thank you for adding these comprehensive e2e tests for the library page! The test coverage looks excellent, with tests for all major functionality including navigation, searching, pagination, sorting, and uploading. Before this PR can be merged, there are a few items that need to be addressed: Required Changes
Questions/Suggestions
Your test implementation looks thorough and well-structured. I particularly appreciate the comprehensive approach to testing edge cases and error handling scenarios. |
Thank you for adding these comprehensive tests for the library page! The test suite looks well-designed with good coverage of normal flows and edge cases. Items to address before merging:
Otherwise, the tests look well-structured and comprehensive. I appreciate the detailed test cases covering normal flows and edge cases for the library page functionality. |
Thanks for this comprehensive set of e2e tests for the library page! The tests look well-structured and cover a good range of functionality including navigation, loading, agent visibility, pagination, sorting, searching, and uploads. A couple of notes on your PR:
Overall, excellent work on the tests themselves! Once you address the API changes (either by justifying their inclusion or moving them to a separate PR), this should be ready to merge. |
Thanks for adding these comprehensive e2e tests for the library page! This is a valuable addition to our test suite that will help catch regressions. I have two concerns about the PR that need to be addressed:
Otherwise, the test implementation looks thorough and well-structured. I appreciate the comprehensive test coverage for different library page functionality including navigation, loading, agent cards, pagination, sorting, searching, and edge cases. Please address these concerns, and we'll be able to move forward with this PR. |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
In this PR, I’ve added library page tests.
Changes
I’ve added 9 tests: 8 for normal flows and 1 for checking edge cases.
Test names are something like:
Other than that, I’ve added a new utility that uses the build page to help us create users at the start, which we could use to test the library page.
Checklist 📋
For code changes: