Move integration tests into integration dir #649
Conversation
WalkthroughIntegration test files were moved from the main package to a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Error as Error Struct
participant Encoder as Encoder (mockEncoder/failEncoder)
Test->>Error: Set Encoder (mockEncoder/failEncoder)
Test->>Error: Call ErrorBody()
Error->>Encoder: Decode(error body bytes, MeilisearchApiError)
alt Successful decode (mockEncoder)
Encoder-->>Error: Populate MeilisearchApiError with mock values
Error-->>Test: Return populated MeilisearchApiError
else Failed decode (failEncoder)
Encoder-->>Error: Return error
Error-->>Test: Return empty MeilisearchApiError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #649 +/- ##
==========================================
+ Coverage 84.61% 84.96% +0.35%
==========================================
Files 17 17
Lines 3106 3106
==========================================
+ Hits 2628 2639 +11
+ Misses 344 335 -9
+ Partials 134 132 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR moves integration tests from the main package to a dedicated integration directory structure. The main goals are to reorganize the testing structure and update the makefile to run all tests recursively.
Key changes:
- Moved integration tests from
mainpackage tointegrationpackage - Added proper imports for the
meilisearchpackage in integration tests - Updated makefile to run tests recursively with
go test -v ./... - Added VectorStore experimental feature to type definitions
- Fixed type naming consistency (
meilisearchApiError→APIError)
Reviewed Changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| makefile | Updated test command to recursively run all tests |
| types.go | Added VectorStore experimental feature and renamed APIError type |
| options_test.go | Replaced setup() calls with direct New() calls for unit tests |
| error_test.go | Updated to use new APIError type name |
| error.go | Renamed meilisearchApiError to APIError |
| client.go | Updated to use new APIError type name |
| integration/*.go | Moved integration tests to separate package with proper imports |
![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 (5)
integration/features_test.go (1)
13-15: Consider improving TLS configuration security.The TLS configuration uses
InsecureSkipVerify: truewithout specifying a minimum TLS version, which could allow insecure protocols. While this may be acceptable for testing, consider adding explicit TLS version configuration for better security practices.meilisearch.WithCustomClientWithTLS(&tls.Config{ InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, })Also applies to: 43-45
integration/index_task_test.go (1)
14-16: Consider adding MinVersion to TLS configuration.The TLS configuration is missing a MinVersion setting, which could be a security concern. However, since this is test code with
InsecureSkipVerify: true, it's likely acceptable.If you want to follow security best practices even in tests, consider adding:
meilisearch.WithCustomClientWithTLS(&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, })Also applies to: 95-97, 184-186
integration/index_test.go (1)
12-14: Consider adding MinVersion to TLS configurations.Multiple TLS configurations are missing MinVersion settings. While acceptable for test code with
InsecureSkipVerify: true, adding MinVersion would follow security best practices.Consider adding to all TLS configurations:
&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, }Also applies to: 103-105, 161-163, 219-221, 292-294, 338-340
integration/index_search_test.go (1)
333-335: Consider adding MinVersion to TLS configuration.The TLS configuration is missing MinVersion, consistent with the security concern flagged in other files.
Consider adding:
&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, }integration/meilisearch_test.go (1)
18-102: Basic client tests properly refactored.All types are consistently qualified with the
meilisearch.prefix and the test logic is sound.Note: The TLS configuration uses
InsecureSkipVerify: truewhich is appropriate for integration tests, but consider addingMinVersion: tls.VersionTLS13for better security practices even in test environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client.go(1 hunks)error.go(3 hunks)error_test.go(1 hunks)integration/features_test.go(3 hunks)integration/index_document_test.go(64 hunks)integration/index_search_test.go(36 hunks)integration/index_task_test.go(6 hunks)integration/index_test.go(12 hunks)integration/main_test.go(18 hunks)integration/meilisearch_test.go(120 hunks)makefile(1 hunks)options_test.go(7 hunks)types.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
client.go (1)
error.go (2)
MeilisearchApiError(21-21)APIError(64-69)
error_test.go (1)
error.go (1)
APIError(64-69)
options_test.go (3)
meilisearch.go (1)
New(18-41)options.go (8)
WithCustomClient(50-54)WithCustomClientWithTLS(57-63)WithAPIKey(68-72)WithContentEncoding(79-86)WithCustomRetries(89-102)DisableRetries(105-109)WithCustomJsonMarshaler(119-123)WithCustomJsonUnmarshaler(133-137)enum.go (2)
GzipEncoding(86-86)DefaultCompression(93-93)
integration/features_test.go (2)
options.go (1)
WithCustomClientWithTLS(57-63)meilisearch_interface.go (1)
ServiceManager(8-59)
integration/meilisearch_test.go (5)
options.go (3)
WithCustomClientWithTLS(57-63)WithAPIKey(68-72)WithContentEncoding(79-86)meilisearch_interface.go (1)
ServiceManager(8-59)error.go (5)
Error(73-109)MeilisearchApiError(21-21)APIError(64-69)ErrCode(11-11)MeilisearchCommunicationError(27-27)types.go (18)
KeysQuery(320-323)Key(284-294)Health(539-541)Task(176-189)Details(247-270)TasksQuery(203-218)SwapIndexesParams(534-536)CancelTasksQuery(221-230)DeleteTasksQuery(233-245)IndexConfig(17-22)SearchRequest(356-385)TenantTokenOptions(330-333)MultiSearchRequest(397-400)MultiSearchResponse(430-440)SearchResponse(414-428)MultiSearchFederation(402-407)MultiSearchFederationMergeFacets(409-411)BatchesQuery(620-635)enum.go (14)
TaskStatusSucceeded(43-43)TaskStatus(6-6)TaskStatusEnqueued(39-39)TaskType(4-4)TaskTypeDocumentAdditionOrUpdate(65-65)TaskTypeTaskCancelation(73-73)TaskTypeTaskDeletion(75-75)ContentEncoding(81-81)GzipEncoding(86-86)DefaultCompression(93-93)DeflateEncoding(87-87)BrotliEncoding(88-88)TaskTypeIndexCreation(57-57)TaskTypeIndexDeletion(61-61)
🪛 ast-grep (0.38.6)
integration/features_test.go
[warning] 12-14: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
integration/index_task_test.go
[warning] 13-15: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
integration/meilisearch_test.go
[warning] 336-338: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 575-577: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 649-651: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 1537-1539: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (49)
makefile (1)
4-4: LGTM! Correctly updated for recursive test execution.The addition of
./...to thego testcommand is necessary and correct for running tests in the newintegration/directory structure. This ensures all tests across subdirectories are executed during the test phase.error_test.go (1)
32-32: LGTM! Correctly updated to use the renamed APIError type.The change from
meilisearchApiErrortoAPIErroris consistent with the type rename inerror.goand maintains proper alignment across the codebase.client.go (1)
100-100: LGTM! Correctly updated to use the renamed APIError type.The change from
meilisearchApiErrortoAPIErroris consistent with the type rename inerror.goand maintains proper alignment across the codebase.integration/features_test.go (4)
1-1: LGTM! Correctly updated package declaration for integration tests.The change from
package meilisearchtopackage integrationis correct and aligns with the PR objective of moving integration tests to a dedicated directory.
7-7: LGTM! Correctly added explicit meilisearch package import.The explicit import of the
meilisearchpackage is necessary now that the tests are in a separateintegrationpackage.
13-13: LGTM! Correctly updated type references with package qualification.The updates to use
meilisearch.WithCustomClientWithTLSandmeilisearch.ServiceManagerare correct and necessary for the package reorganization.Also applies to: 19-19, 43-43, 49-49
72-72: LGTM! Minor formatting improvement.The addition of a blank line after the error check improves code readability.
error.go (3)
64-64: LGTM! Improved type naming follows Go conventions.The rename from
meilisearchApiErrortoAPIErrorfollows Go naming conventions and improves code readability.
91-91: LGTM! Correctly updated field type to use renamed APIError.The field type update is consistent with the type rename and maintains proper functionality.
146-146: LGTM! Correctly updated variable declaration to use renamed APIError.The variable declaration update is consistent with the type rename and maintains proper functionality.
options_test.go (1)
12-12: LGTM! Consistent migration fromsetup()toNew()function.The changes consistently replace
setup()calls with directNew()constructor calls, properly passing the host parameter and option functions. This aligns with the updated client instantiation pattern.Also applies to: 23-23, 36-36, 46-46, 57-57, 65-65, 75-75, 84-85
types.go (2)
182-182: LGTM! Error type standardization.The change from
meilisearchApiErrortoAPIErrorimproves naming consistency across the codebase.
521-521: LGTM! New experimental feature flag added.The addition of the
VectorStorefield to both experimental features structs properly supports the new vector store functionality.Also applies to: 531-531
integration/index_task_test.go (2)
1-1: LGTM! Package reorganization implemented correctly.The package name change to
integrationand explicit import of the meilisearch package properly supports the test restructuring.Also applies to: 6-6
20-20: LGTM! Meilisearch types properly qualified.All meilisearch types and constants are correctly qualified with the package prefix, maintaining consistency with the integration package structure.
Also applies to: 83-83, 101-103, 137-141, 151-159, 190-190, 198-198
integration/index_test.go (2)
1-1: LGTM! Package reorganization implemented correctly.The package name change to
integrationand explicit import of the meilisearch package properly supports the test restructuring.Also applies to: 5-5
22-22: LGTM! Meilisearch types properly qualified.All meilisearch types and structs are correctly qualified with the package prefix, maintaining consistency with the integration package structure.
Also applies to: 88-88, 109-109, 114-114, 122-122, 136-136, 166-166, 172-172, 226-226, 231-231, 239-239, 250-250, 298-298, 344-345, 350-350, 356-356, 361-361, 370-370, 375-375
integration/index_search_test.go (2)
1-1: LGTM! Package reorganization implemented correctly.The package name change to
integrationand explicit import of the meilisearch package properly supports the test restructuring.Also applies to: 6-6
15-21: LGTM! Comprehensive type qualification completed.All meilisearch types throughout the file are correctly qualified with the package prefix, including SearchRequest, SearchResponse, FacetSearchRequest, FacetSearchResponse, ContentEncoding variants, and other related types. The changes maintain consistency with the integration package structure.
Also applies to: 24-41, 53-79, 82-108, 113-113, 127-127, 139-139, 153-156, 161-162, 170-175, 215-217, 251-254, 258-261, 278-281, 342-343, 348-349, 368-374, 396-402, 424-430, 504-505, 509-510, 519-524, 540-545, 601-604, 608-609, 682-683, 689-690, 771-772, 776-777, 809-810, 813-814, 833-834, 838-839, 896-896, 903-903, 935-935, 942-942, 971-974, 980-987, 1018-1021, 1026-1028, 1052-1055, 1060-1065, 1070-1075, 1104-1105, 1111-1112, 1119-1126, 1148-1149, 1160-1167, 1199-1201
integration/index_document_test.go (8)
1-9: LGTM! Clean package refactoring.The package declaration change from
meilisearchtointegrationand the explicit import of the meilisearch package follows Go best practices for organizing integration tests in a separate package.
11-131: Consistent type qualification throughout test cases.All Meilisearch types (
ContentEncoding,TaskInfo,DocumentsResult,Hits, etc.) are properly qualified with themeilisearch.prefix. The test logic for content encoding variations is well-structured and comprehensive.
133-232: Well-structured document operation tests.The test cases properly cover different document ID scenarios and all types are consistently qualified with the
meilisearch.prefix. The test logic and assertions are appropriate.
234-328: Primary key tests properly refactored.All types are consistently qualified and the test logic for primary key functionality is sound.
330-486: Comprehensive batch operations testing.The batch operation tests cover both scenarios (with and without primary keys) and all types are consistently qualified with the
meilisearch.prefix.
488-896: Document format tests thoroughly refactored.All NDJSON and CSV document tests are properly refactored with consistent type qualification. The tests appropriately cover both string and reader input variations.
1010-1464: Delete operations tests properly refactored.All delete operation tests maintain consistent type qualification and cover appropriate scenarios including custom TLS configurations.
1466-1497: Experimental feature test properly refactored.The UpdateDocumentsByFunction test correctly uses qualified types and properly handles experimental feature setup.
integration/meilisearch_test.go (10)
1-16: Consistent package refactoring.The package declaration and import changes are consistent with the refactoring pattern, properly establishing the integration test package with explicit meilisearch imports.
104-523: Key management tests thoroughly refactored.All key management operations are properly tested with consistent type qualification throughout. The test coverage for key operations is comprehensive.
525-730: Health and task tests properly refactored.All health and task-related tests maintain consistent type qualification and appropriate test logic.
732-1123: Task management tests comprehensively refactored.All task management tests maintain consistent type qualification and provide thorough coverage of various query scenarios.
1124-1535: Task operations tests properly refactored.All task operation tests maintain consistent type qualification and provide good coverage of both success and failure scenarios.
1536-1735: Wait for task tests properly refactored.All wait-for-task tests maintain consistent type qualification and provide good coverage of timeout and connection scenarios.
1737-1965: Tenant token tests thoroughly refactored.All tenant token tests maintain consistent type qualification and provide comprehensive coverage of various tenant token scenarios.
1967-2324: Multi-search tests comprehensively refactored.All multi-search tests maintain consistent type qualification and provide thorough coverage of complex federation scenarios.
2326-2571: Index management tests properly refactored.All index management tests maintain consistent type qualification and provide appropriate coverage of index operations.
2573-2640: Batch management tests properly refactored.All batch management tests maintain consistent type qualification and provide appropriate coverage of batch operations.
integration/main_test.go (12)
1-1: Package refactoring looks good.The package name change from
meilisearchtointegrationand the addition of the explicit import for the meilisearch package are correctly implemented and align with the PR objectives to organize integration tests in a dedicated directory.Also applies to: 8-8
21-38: Type qualifications are correctly applied.All meilisearch types are properly qualified with the
meilisearch.prefix, maintaining the same functional behavior while adapting to the new package structure.
68-83: Function signature updates are consistent.The parameter types and function calls are correctly updated to use fully qualified meilisearch types, maintaining the same functionality while adapting to the new package structure.
83-151: Helper function updates are properly implemented.All function signatures consistently use fully qualified meilisearch types while preserving the original functionality. The changes align with the package refactoring objectives.
161-173: Test helper functions correctly updated.The type qualifications for
meilisearch.IndexManager,meilisearch.TaskInfo, andmeilisearch.TaskStatusSucceededare properly applied while maintaining the original test helper functionality.
175-235: Index setup functions properly refactored.All function signatures and return types are correctly updated to use fully qualified meilisearch types, maintaining the original functionality while adapting to the new package structure.
237-297: Additional setup functions correctly updated.The type qualifications for
meilisearch.ServiceManagerandmeilisearch.IndexManagerare properly applied while preserving the original functionality of these helper functions.
299-320: Nested fields setup function correctly updated.The function signature is properly updated to use
meilisearch.ServiceManagerwhile maintaining the original functionality.
322-359: Vector setup function comprehensively updated.All meilisearch types are properly qualified including
meilisearch.Settings,meilisearch.Embedder, andmeilisearch.UserProvidedEmbedderSource. The error message formatting usingfmt.Errorfis also correctly applied.
361-393: Distinct index setup function correctly updated.The function signature is properly updated to use
meilisearch.ServiceManagerwhile preserving the original functionality.
420-432: Utility function correctly updated.The function signature is properly updated to use
meilisearch.Hitswhile maintaining the original functionality.
434-449: NDJSON parsing function correctly updated.The function signature and variable declarations are properly updated to use
meilisearch.Hitsandmeilisearch.Hitwhile preserving the original parsing functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)codecov.yml(0 hunks)
💤 Files with no reviewable changes (1)
- codecov.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (go latest version)
- GitHub Check: integration-tests (go current version)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
41-46: EOL Go 1.16 (and linter on 1.17) still in the matrix – please drop / bump
This is the same concern raised in the earlier review: Go 1.16 is end-of-life and lacks generics support; 1.17 (hard-coded in the linter job) is also EOL. Keeping them burns CI time and will explode when any dependency starts using generics.Actionable diff:
- go: [1.16, 1.24] + # CI runs on maintained Go releases only + go: [1.22, 1.24] ... - go-version: 1.17 + go-version: 1.22(Tag the first entry in the matrix as
currentif you rely on that.)Remember to update any other
setup-gosteps that still pin 1.17.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
33-35: Good guard-rail – tests now wait for a clean lint passLinking
integration_teststo thelinterjob prevents wasting CI minutes on compilation & Docker work when the code doesn’t even pass static analysis.
Looks correct as written.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pre-release-tests.yml(1 hunks).github/workflows/tests.yml(2 hunks)types_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
types_test.go (1)
types.go (3)
SearchRequest(356-385)SearchRequestHybrid(392-395)Embedder(103-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (go latest version)
- GitHub Check: integration-tests (go current version)
🔇 Additional comments (1)
types_test.go (1)
61-84: validate() behavior confirmed and tests approvedThe
validate()method onSearchRequest(types.go:548–552) correctly defaults an emptyHybrid.Embedderto"default"and preserves custom values. The new subtests intypes_test.go(lines 61–84) are well-structured, uset.Parallel(), and cover all intended scenarios.• types.go:548–552 —
func (s *SearchRequest) validate() { … }
• types_test.go:61–84 —TestSearchRequest_validatesubtestsOptional enhancement:
- Add a subtest to ensure other fields (e.g.,
SemanticRatio,Query) remain unchanged byvalidate().
curquiza
added
the
maintenance
There was a problem hiding this comment.
Can you please remove everything that is not related to the PR, meaning moving integration tests to another folder
this PR is only about maintenance, not breaking change on the API or anything.
It will be more transparent and easy to understand for users, thank you 🙏
Also there is some git conflicts
Thank you again for your PR
# Conflicts: # integration/meilisearch_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
integration/index_task_test.go (1)
14-16: Consider improving TLS security configuration.The TLS configuration uses
InsecureSkipVerify: truewithout specifying a minimum TLS version, which could allow insecure TLS versions by default.While this is acceptable for integration tests, consider adding
MinVersion: tls.VersionTLS12for better security practices:meilisearch.WithCustomClientWithTLS(&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, })Also applies to: 95-97, 184-186
integration/meilisearch_test.go (1)
20-22: Address TLS security configuration across multiple test cases.Multiple TLS configurations throughout the file use
InsecureSkipVerify: truewithout specifying minimum TLS version. While acceptable for integration tests, consider standardizing the configuration for better security practices:meilisearch.WithCustomClientWithTLS(&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, })Also applies to: 76-78, 104-106, 139-141, 212-214, 335-337, 433-435, 525-527, 574-576, 648-650, 732-734, 898-900, 1536-1538, 1601-1603
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/tests.yml(1 hunks)error_test.go(2 hunks)integration/index_document_test.go(66 hunks)integration/index_task_test.go(6 hunks)integration/main_test.go(18 hunks)integration/meilisearch_test.go(120 hunks)types.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/tests.yml
- types.go
- error_test.go
- integration/index_document_test.go
- integration/main_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
integration/meilisearch_test.go (5)
options.go (3)
WithCustomClientWithTLS(57-63)WithAPIKey(68-72)WithContentEncoding(79-86)meilisearch_interface.go (1)
ServiceManager(8-59)types.go (18)
KeysQuery(321-324)Key(285-295)Health(542-544)Task(177-190)Details(248-271)TasksQuery(204-219)SwapIndexesParams(537-539)CancelTasksQuery(222-231)DeleteTasksQuery(234-246)IndexConfig(17-22)SearchRequest(357-386)TenantTokenOptions(331-334)MultiSearchRequest(398-401)MultiSearchResponse(431-441)SearchResponse(415-429)MultiSearchFederation(403-408)MultiSearchFederationMergeFacets(410-412)BatchesQuery(623-638)enum.go (14)
TaskStatusSucceeded(43-43)TaskStatus(6-6)TaskStatusEnqueued(39-39)TaskType(4-4)TaskTypeDocumentAdditionOrUpdate(65-65)TaskTypeTaskCancelation(73-73)TaskTypeTaskDeletion(75-75)ContentEncoding(81-81)GzipEncoding(86-86)DefaultCompression(93-93)DeflateEncoding(87-87)BrotliEncoding(88-88)TaskTypeIndexCreation(57-57)TaskTypeIndexDeletion(61-61)error.go (3)
Error(73-109)ErrCode(11-11)MeilisearchCommunicationError(27-27)
🪛 ast-grep (0.38.6)
integration/index_task_test.go
[warning] 13-15: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
integration/meilisearch_test.go
[warning] 334-336: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 573-575: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 647-649: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 1535-1537: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (10)
integration/index_task_test.go (4)
1-1: LGTM: Package reorganization is well-executed.The systematic change from
meilisearchpackage tointegrationpackage with explicit imports properly separates integration tests from the main package.
6-6: LGTM: Proper external package import.Adding the explicit import ensures clear separation between the test package and the Meilisearch client library.
20-20: LGTM: Proper type qualification.All
ServiceManagertypes are correctly qualified with themeilisearchpackage prefix.Also applies to: 101-101, 190-190
83-83: LGTM: Consistent constant and type qualification.All Meilisearch constants and types are properly qualified with the package prefix, ensuring clear external API usage.
Also applies to: 138-140, 155-157, 198-198
integration/meilisearch_test.go (6)
1-1: LGTM: Excellent package reorganization.The systematic refactoring from
meilisearchtointegrationpackage demonstrates proper test organization and clear separation of concerns.
7-7: LGTM: Proper external package import.The explicit import of the meilisearch package ensures clear API boundaries between test and library code.
26-26: LGTM: Comprehensive type qualification.All Meilisearch types including
ServiceManager,Key,KeysQuery,Health,Task,TasksQuery,CancelTasksQuery,DeleteTasksQuery, and others are properly qualified with the package prefix. This ensures clear external API usage throughout the test suite.Also applies to: 53-53, 82-82, 112-112, 144-144, 218-219, 341-344, 439-440, 533-534, 582-582, 614-615, 654-655, 738-739, 904-905, 974-975, 1127-1128, 1207-1208, 1340-1341, 1471-1472, 1542-1543, 1607-1607
706-706: LGTM: Improved variable naming for clarity.The consistent renaming from
tasktotaskInfothroughout the file improves code readability and reduces ambiguity about the variable's purpose.Also applies to: 871-871, 1093-1093, 1589-1589, 1696-1696
640-640: LGTM: Consistent constant qualification.All task status constants and other Meilisearch enums are properly qualified with the package prefix, maintaining consistency across the test suite.
Also applies to: 718-718, 1328-1328, 1459-1459, 1529-1529, 2380-2380, 2442-2442, 2499-2499, 2511-2511, 2545-2545, 2551-2551, 2582-2582, 2588-2588
1172-1174: LGTM: Proper struct initialization with qualified types.All struct initializations using Meilisearch types (Key, IndexConfig, SwapIndexesParams, etc.) are correctly qualified, ensuring proper external API usage.
Also applies to: 1180-1182, 1194-1196, 1507-1509, 1875-1875, 1955-1955, 2363-2363, 2431-2433, 2490-2492, 2538-2540, 2575-2577
# Conflicts: # integration/index_document_test.go # integration/index_search_test.go # integration/index_settings_test.go # integration/meilisearch_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
encoding_test.go (1)
33-49: Review the mockEncoder implementation for testing completeness.The
mockEncoderimplementation looks correct for testing purposes, but consider the following:
- The
Encodemethod returns(nil, nil)unconditionally, which may not adequately test error conditions or successful encoding paths.- The
Decodemethod specifically targets*meilisearchApiErrortype, which should be documented or the type should be visible in this test file.Consider adding comments to clarify the intended test scenarios:
+// mockEncoder is a test encoder that simulates specific behaviors for testing type mockEncoder struct{} +// Encode always returns nil to simulate a no-op encoding for testing func (m *mockEncoder) Encode(r io.Reader) (io.ReadCloser, error) { return nil, nil } +// Decode populates meilisearchApiError with mock data for testing func (m *mockEncoder) Decode(data []byte, v interface{}) error {integration/meilisearch_test.go (1)
20-22: Consider setting MinVersion for TLS configuration security.The TLS configuration lacks a MinVersion setting, which could allow insecure TLS versions. While this is test code, it's good practice to use secure configurations.
Consider adding MinVersion to improve security:
customSv := setup(t, "", meilisearch.WithCustomClientWithTLS(&tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, }))This pattern appears in multiple test functions throughout the file (lines 76-78, 104-106, 139-141, etc.) and should be consistently applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
encoding_test.go(2 hunks)error_test.go(2 hunks)integration/index_document_test.go(70 hunks)integration/index_search_test.go(36 hunks)integration/main_test.go(18 hunks)integration/meilisearch_test.go(120 hunks)types.go(3 hunks)
🪛 ast-grep (0.38.6)
integration/meilisearch_test.go
[warning] 334-336: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 573-575: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 647-649: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 1535-1537: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
✅ Files skipped from review due to trivial changes (1)
- integration/index_search_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- error_test.go
- types.go
- integration/index_document_test.go
- integration/main_test.go
🧰 Additional context used
🪛 ast-grep (0.38.6)
integration/meilisearch_test.go
[warning] 334-336: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 573-575: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 647-649: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 1535-1537: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (go current version)
- GitHub Check: integration-tests (go latest version)
🔇 Additional comments (4)
encoding_test.go (2)
7-7: LGTM: Import added for mockEncoder error handling.The
fmtimport is correctly added to support thefmt.Errorfcall in the newmockEncoder.Decodemethod.
1-1: No action needed for encoding_test.go packageThe
encoding_test.gofile resides in the root of the repository alongside other unit tests (e.g.client_test.go,error_test.go) and correctly declarespackage meilisearch. Only tests within theintegration/directory usepackage integration, which matches the project’s structure after the PR changes. You can ignore the earlier flag.Likely an incorrect or invalid review comment.
integration/meilisearch_test.go (2)
1-16: LGTM! Clean package refactoring.The package declaration change from
meilisearchtointegrationand the addition of the explicit meilisearch import are correctly implemented. This allows for proper separation of integration tests while maintaining access to all required types and functions.
26-2667: Excellent consistent refactoring of type qualifications.All meilisearch types, constants, and functions have been consistently updated to use explicit
meilisearch.package qualification throughout the file. This includes:
- Service and client types (
meilisearch.ServiceManager)- Data types (
meilisearch.Key,meilisearch.Task,meilisearch.SearchRequest, etc.)- Constants (
meilisearch.TaskStatusSucceeded,meilisearch.MeilisearchCommunicationError)- Enums (
meilisearch.ContentEncoding,meilisearch.TaskStatus)The refactoring is comprehensive and maintains full functionality while properly separating the integration tests into their own package.
|
bors merge |
curquiza
changed the title
|
bors merge |
|
bors is bugging again, I'm merging by hand
|
Pull Request
Related issue
Fixes #648
What does this PR do?
integrationdirectory.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores