chore(server): refactoring e2e test code #1393
Conversation
WalkthroughThe pull request introduces significant changes to the end-to-end (e2e) testing infrastructure in the server directory. The modifications primarily focus on simplifying server initialization, streamlining GraphQL request handling, and refactoring test code across multiple test files. The changes include introducing a new Changes
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/e2e/common.go (2)
127-128: Consider capturing the ignored return values if needed.
You are ignoringgateway.Containerandaccountgateway.ContainerfromStartGQLServerWithRepos. If they are not required for these tests, this is fine. If future tests need these, consider capturing them.
183-185: Consider capturing ignored return values.
Similar to above, ifrepo.Containerorgateway.Containermight be needed by other tests or future logic, capture them. Otherwise, this is acceptable for a simple test context.server/e2e/gql_project_export_test.go (1)
Line range hint
95-106: Consider improving file handling in exporProject function.The current implementation could be enhanced for better error handling and resource cleanup:
- Use
deferimmediately after file creation- Consider using
os.CreateTemp()for safer temporary file handlingfunc exporProject(t *testing.T, e *httpexpect.Expect, p string) string { // ... existing code ... - fileName := "project_data.zip" - err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) - assert.Nil(t, err) - return fileName + tmpFile, err := os.CreateTemp("", "project_data_*.zip") + assert.Nil(t, err) + fileName := tmpFile.Name() + defer tmpFile.Close() + + _, err = tmpFile.Write([]byte(downloadResponse)) + assert.Nil(t, err) + + return fileName }server/e2e/gql_asset_test.go (1)
Line range hint
115-146: Consider improving test file cleanup in createAsset function.The current implementation could benefit from more robust cleanup of test files:
- Use
t.Cleanup()for guaranteed cleanup even if tests fail- Add error handling for file operations
func createAsset(t *testing.T, e *httpexpect.Expect, filePath string, coreSupport bool, teamId string) *httpexpect.Value { file, err := os.Open(filePath) if err != nil { t.Fatalf("failed to open file: %v", err) } - defer func() { - if cerr := file.Close(); cerr != nil && err == nil { - err = cerr - } - }() + t.Cleanup(func() { + if err := file.Close(); err != nil { + t.Errorf("failed to close file: %v", err) + } + }) // ... rest of the function }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
server/e2e/common.go(5 hunks)server/e2e/dataset_export_test.go(1 hunks)server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_featureCollection_test.go(3 hunks)server/e2e/gql_me_test.go(1 hunks)server/e2e/gql_project_export_test.go(3 hunks)server/e2e/gql_project_import_test.go(2 hunks)server/e2e/gql_project_test.go(6 hunks)server/e2e/gql_style_test.go(5 hunks)server/e2e/mock_test.go(1 hunks)
🧰 Additional context used
📓 Learnings (2)
server/e2e/gql_project_import_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
server/e2e/gql_project_export_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (24)
server/e2e/common.go (7)
66-70: No issues found withinitAccountGateway.
The function appears straightforward and well-structured, returning a functional mailer container without unnecessary complexity.
72-82: Server initialization logic looks good.
Returning theWebServer,gateway.Container, andaccountgateway.Containerenhances modularity. No further concerns.
Line range hint
85-116: Concurrent server startup approach is appropriate.
Using a goroutine and channel to handle server shutdown is standard for test scenarios. Implementation is clear and error handling is properly addressed.
131-140: Initialization logic is consistent.
initServercorrectly encapsulates server setup. This maintains clarity and consistency across the codebase.
142-142: General startup logic is standard for test environments.
Short-mode skip, background context creation, and listening on a random port help ensure clean and isolated test runs.Also applies to: 146-146, 150-150, 156-156, 174-174
177-181: Chaining server and repo initialization is clear.
StartServerAndReposeffectively delegates toStartServerWithRepos. The approach is straightforward and easy to maintain.
212-216: GraphQL request struct is well-defined.
Usinganyis fine in Go 1.18+ for unstructured JSON variables, and the overall structure is clean and consistent.server/e2e/gql_me_test.go (1)
24-24: Request utility usage is clear.
LeveragingRequest(e, uID.String(), requestBody)simplifies the test and helps standardize request creation across scenarios.server/e2e/mock_test.go (1)
44-44: Refactor toRequest(...)is an improvement.
Replacing manual POST setup with a dedicated helper function centralizes request logic and enhances maintainability.server/e2e/dataset_export_test.go (1)
11-11: LGTM! Server initialization refactored successfully.The simplified server initialization maintains test functionality while reducing complexity.
server/e2e/gql_project_export_test.go (1)
18-18: LGTM! Server initialization refactored successfully.The simplified server initialization maintains test functionality while reducing complexity.
server/e2e/gql_style_test.go (1)
52-52: LGTM! Request handling refactored consistently.The changes successfully implement the new Request() pattern across all GraphQL operations while maintaining comprehensive test coverage.
Also applies to: 76-76, 94-94, 116-116, 143-143, 148-148
server/e2e/gql_asset_test.go (1)
218-218: LGTM! Request handling refactored successfully.The change successfully implements the new Request() pattern for asset queries.
server/e2e/gql_project_import_test.go (2)
80-80: LGTM!The function has been refactored to use the new
Requestfunction while maintaining proper response validation.
17-17: Verify if assertions are affected by GraphQL problems.Based on the retrieved learnings from similar test files, assertions in GraphQL-related tests were disabled due to existing problems. Please verify if the assertions in this test are also affected by these GraphQL problems.
✅ Verification successful
Assertions are active and functional
The test contains multiple active assertions that are being executed, unlike the disabled assertions mentioned in the learnings. The assertions are properly checking the response structure and values, indicating they are not affected by the GraphQL problems mentioned in other test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for disabled assertions in GraphQL test files. # Test: Search for commented assertions or disabled test cases. rg -A 5 '(//\s*assert|//\s*t\.Assert|//\s*r\.Value|//\s*r\.Object|//\s*r\.Array|//\s*r\.Equal|//\s*r\.NotNull|//\s*r\.True|//\s*r\.False)' server/e2e/gql_*.goLength of output: 670
Script:
#!/bin/bash # Get the implementation of TestCallImportProject and surrounding context rg -B 2 -A 10 "func TestCallImportProject" server/e2e/gql_project_import_test.go # Also check for any assertion patterns in the file rg "assert|r\.(Value|Object|Array|Equal|NotNull|True|False)" server/e2e/gql_project_import_test.goLength of output: 935
server/e2e/gql_featureCollection_test.go (4)
73-73: LGTM!The function has been refactored to use the new
Requestfunction while maintaining proper response handling.
144-144: LGTM!The function has been refactored to use the new
Requestfunction while maintaining proper response handling.
170-170: LGTM!The function has been refactored to use the new
Requestfunction while maintaining proper response handling.
177-177: LGTM!The server initialization has been simplified using the new
Serverfunction.server/e2e/gql_project_test.go (5)
20-20: LGTM!The server initialization has been simplified using the new
Serverfunction.
57-57: LGTM!The server initialization has been simplified using the new
Serverfunction.
155-155: LGTM!The server initialization has been simplified using the new
Serverfunction.
309-309: LGTM!The server initialization has been simplified using the new
Serverfunction.
454-454: LGTM!The function has been refactored to use the new
Requestfunction while maintaining proper response handling.
hexaforce
force-pushed
the
chore/server-e2e-test-refactoring
branch
2 times, most recently
from
a10fe49 to
84bf996
Compare
hexaforce
force-pushed
the
chore/server-e2e-test-refactoring
branch
from
fe07980 to
b957ac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/e2e/common.go (3)
66-70: Consider making the mailer config configurable.The function initializes a mailer with an empty config, which might not be suitable for all test scenarios. Consider accepting a mailer config parameter to make the function more flexible.
-func initAccountGateway(ctx context.Context) *accountgateway.Container { +func initAccountGateway(ctx context.Context, mailerConfig *mailer.Config) *accountgateway.Container { return &accountgateway.Container{ - Mailer: mailer.New(ctx, &mailer.Config{}), + Mailer: mailer.New(ctx, mailerConfig), } }
72-83: Consider making debug mode configurable.The function hardcodes debug mode to true. Consider accepting a debug parameter to make the function more flexible for different test scenarios.
-func initServerWithAccountGateway(cfg *config.Config, repos *repo.Container, ctx context.Context) (*app.WebServer, *gateway.Container, *accountgateway.Container) { +func initServerWithAccountGateway(cfg *config.Config, repos *repo.Container, ctx context.Context, debug bool) (*app.WebServer, *gateway.Container, *accountgateway.Container) { gateways := initGateway() accountGateway := initAccountGateway(ctx) return app.NewServer(ctx, &app.ServerConfig{ Config: cfg, Repos: repos, AccountRepos: repos.AccountRepos(), Gateways: gateways, AccountGateways: accountGateway, - Debug: true, + Debug: debug, }), gateways, accountGateway }
131-140: Consider code reuse and making debug mode configurable.The function shares similar code with
initServerWithAccountGateway. Consider:
- Making debug mode configurable
- Reusing code to avoid duplication
-func initServer(cfg *config.Config, repos *repo.Container, ctx context.Context) (*app.WebServer, *gateway.Container) { +func initServer(cfg *config.Config, repos *repo.Container, ctx context.Context, debug bool) (*app.WebServer, *gateway.Container) { - gateways := initGateway() - return app.NewServer(ctx, &app.ServerConfig{ - Config: cfg, - Repos: repos, - AccountRepos: repos.AccountRepos(), - Gateways: gateways, - Debug: true, - }), gateways + srv, gateways, _ := initServerWithAccountGateway(cfg, repos, ctx, debug) + return srv, gateways }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
server/e2e/common.go(5 hunks)server/e2e/dataset_export_test.go(1 hunks)server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_featureCollection_test.go(3 hunks)server/e2e/gql_me_test.go(1 hunks)server/e2e/gql_project_export_test.go(3 hunks)server/e2e/gql_project_import_test.go(2 hunks)server/e2e/gql_project_test.go(6 hunks)server/e2e/gql_style_test.go(5 hunks)server/e2e/mock_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- server/e2e/mock_test.go
- server/e2e/gql_me_test.go
- server/e2e/gql_asset_test.go
- server/e2e/dataset_export_test.go
- server/e2e/gql_project_import_test.go
- server/e2e/gql_featureCollection_test.go
- server/e2e/gql_project_export_test.go
- server/e2e/gql_style_test.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (3)
server/e2e/common.go (2)
85-117: LGTM!The function has been improved to return additional components (gateways and accountGateway) while maintaining robust error handling and proper cleanup.
212-216: LGTM!The struct is well-defined with proper JSON tags for GraphQL operations.
server/e2e/gql_project_test.go (1)
20-20: LGTM! Code refactoring improves maintainability.The changes improve code maintainability by:
- Simplifying server initialization using the
Serverfunction- Standardizing request handling using the new
RequestfunctionAlso applies to: 57-57, 155-155, 367-367, 454-454
Overview
refined the test code.
What I've done
I consolidated redundant processes to reduce the amount of code.
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Refactoring
Requestfunction for making GraphQL requests.Test Improvements
Code Cleanup
These changes primarily focus on improving the internal testing infrastructure without introducing user-facing changes.