added yaml rest integration test & document-indexing test#20691
added yaml rest integration test & document-indexing test#20691ThyTran1402 wants to merge 3 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
github-actions
bot
added
bug
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTest additions validating that documents can be indexed when a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml (1)
1244-1266: No teardown fortest-term-vector-index— adds retry/rerun fragility.
test-term-vector-indexis created inside a named test section, not in the globalsetup, so there is no corresponding automatic cleanup. Thesetupsection runs before each test section in order to set up the same environment for each test section. Without a teardown, a second execution within the same cluster (e.g., retry on failure, parallel suites) will hit aresource_already_exists_exceptiononindices.create.A
teardownsection can clean up resources created during the setup or test actions. Consider one of:
- Adding an
indices.deleteat the top of this test section withignore: 404(defensive delete before create).- Promoting index creation to the global
setupand deletion to a file-levelteardown.💡 Minimal fix — guard at the top of the test section
"index document with term_vector on search_as_you_type field": + - do: + indices.delete: + index: test-term-vector-index + ignore_unavailable: true + - do: indices.create: index: test-term-vector-index🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml` around lines 1244 - 1266, The test creates an index named test-term-vector-index inside the test section but never deletes it, which causes flaky retries/resource_already_exists exceptions; fix by adding a defensive indices.delete action for test-term-vector-index with ignore: 404 at the start of this test section (or alternatively move the indices.create into the global setup and add a corresponding file-level teardown indices.delete), ensuring the index is removed before creation so subsequent runs don't fail.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/mapper-extras/src/test/java/org/opensearch/index/mapper/SearchAsYouTypeFieldMapperTests.javamodules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.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). (1)
- GitHub Check: gradle-check
🔇 Additional comments (1)
modules/mapper-extras/src/test/java/org/opensearch/index/mapper/SearchAsYouTypeFieldMapperTests.java (1)
385-388: LGTM — correctly exercises the LuceneFieldType.validate()path.
mapper.parse()is exactly the right call site: LuceneFieldobjects (which internally invokeFieldType.validate()) are instantiated during document parsing. Without the fix, the illegal combination ofstoreTermVectorOffsets=true/storeTermVectors=falseon_index_prefixwould throwIllegalStateExceptionat this point. The discarded return value is idiomatic for a "must not throw" assertion. Running this check inside the existing loop ensures all six term-vector variants are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml`:
- Around line 1244-1266: The test "index document with term_vector on
search_as_you_type field" creates a persistent index named
test-term-vector-index and lacks cleanup; add a teardown step that calls
indices.delete for test-term-vector-index (e.g., append a do: indices.delete
block targeting index: test-term-vector-index) either inside this test section
or as a file-level teardown to ensure the index is removed after the test.
---
Nitpick comments:
In
`@modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml`:
- Around line 1244-1266: The test creates an index named test-term-vector-index
inside the test section but never deletes it, which causes flaky
retries/resource_already_exists exceptions; fix by adding a defensive
indices.delete action for test-term-vector-index with ignore: 404 at the start
of this test section (or alternatively move the indices.create into the global
setup and add a corresponding file-level teardown indices.delete), ensuring the
index is removed before creation so subsequent runs don't fail.
| --- | ||
| "index document with term_vector on search_as_you_type field": | ||
|
|
||
| - do: | ||
| indices.create: | ||
| index: test-term-vector-index | ||
| body: | ||
| settings: | ||
| number_of_replicas: 0 | ||
| mappings: | ||
| properties: | ||
| my_field: | ||
| type: search_as_you_type | ||
| term_vector: with_positions_offsets | ||
|
|
||
| - do: | ||
| index: | ||
| index: test-term-vector-index | ||
| id: 1 | ||
| body: | ||
| my_field: "hello world" | ||
|
|
||
| - match: { result: created } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for duplicate occurrences of the test scenario name across YAML test files
rg -rn "index document with term_vector on search_as_you_type field" --type yamlRepository: opensearch-project/OpenSearch
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Find the actual file and examine its structure
find . -name "10_basic.yml" -path "*search-as-you-type*" -type fRepository: opensearch-project/OpenSearch
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Get file size and line count
wc -l "modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml" 2>/dev/null || echo "File not found"Repository: opensearch-project/OpenSearch
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Read the end of the file to see the test in question and any teardown
tail -50 "modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml"Repository: opensearch-project/OpenSearch
Length of output: 1403
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of "term_vector" and "search_as_you_type" together in the file
rg -n "term_vector|teardown|---" "modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml" | head -100Repository: opensearch-project/OpenSearch
Length of output: 793
🏁 Script executed:
#!/bin/bash
# Simple grep to find if the test scenario name appears more than once
grep -n "index document with term_vector" "modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml"Repository: opensearch-project/OpenSearch
Length of output: 140
Add a teardown section to clean up the test-term-vector-index created in this test.
This test creates a persistent index (test-term-vector-index) at line 1251 but does not delete it. Since this is the final test in the file and there is no file-level teardown, the index remains in the cluster. Add either a teardown block within this test section or a file-level teardown to delete the index after tests complete:
- do:
indices.delete:
index: test-term-vector-index
This prevents state pollution and ensures the test can be re-run without encountering a 409 Conflict error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/10_basic.yml`
around lines 1244 - 1266, The test "index document with term_vector on
search_as_you_type field" creates a persistent index named
test-term-vector-index and lacks cleanup; add a teardown step that calls
indices.delete for test-term-vector-index (e.g., append a do: indices.delete
block targeting index: test-term-vector-index) either inside this test section
or as a file-level teardown to ensure the index is removed after the test.
|
❌ Gradle check result for fe7d0e4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
|
❌ Gradle check result for 73dcab6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
|
||
| - match: { hits.total: 1 } | ||
| - match: { hits.hits.0._source.a_field: "quick brown fox jump lazy dog" } | ||
|
|
There was a problem hiding this comment.
Was the issue resolved by #3119? So this PR aims to add more tests?
There was a problem hiding this comment.
Yup. The issue fixed by PR #3119. This PR only added test to verifies actual document indexing doesn't throw, and not just that FieldType flags are set correctly. They close a gap in the existing test coverage so we could regress the production fix without the existing tests catching the actual exception.
Let me know what your thoughts about it?
Thank you!
|
Hi @gaobinlong I think the PR is ready for review. Thank you! |
|
❌ Gradle check result for 73dcab6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
|
Hi @gaobinlong I just checked again and realized that the bug was not fully fixed it yet. And the bug is _index_prefix subfield does not call setTermVectorParams, but there may be a code path possibly through Lucene's segment merging or the test framework's document writer where term vector metadata from the sibling shingle fields bleeds into the prefix field's FieldInfo, causing a conflict. So, I fixed it recently, let me know if that works. Can you review it again please? Thank you! |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
❌ Gradle check result for cb6eff4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
2 participants
Description
Related Issues
Resolves #1901
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.