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

Use different package name between restart/rolling bwc tests #1096

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

heemin32
Copy link
Collaborator

Description

During bwc test debugging, intelliJ is pointing to wrong code part becase restart/rolling bwc tests are sharing same package name and class name. Separating the package name between the two so that intelliJ debugger can point to correct code part.

Related Issues

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@heemin32 heemin32 added Refactoring Improve the design, structure, and implementation while preserving its functionality skip-changelog labels Jan 14, 2025
@heemin32 heemin32 added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Jan 14, 2025
@yuye-aws
Copy link
Member

@heemin32 It's preferred to keep restart and rolling package consistent.
package org.opensearch.neuralsearch.bwc.restart; package org.opensearch.neuralsearch.bwc.rolling;

@yuye-aws
Copy link
Member

The code looks good to me. Just need @heemin32 to fix the comment above.

@heemin32
Copy link
Collaborator Author

@heemin32 It's preferred to keep restart and rolling package consistent. package org.opensearch.neuralsearch.bwc.restart; package org.opensearch.neuralsearch.bwc.rolling;

Thanks. It was a mistake.

Copy link
Member

@zhichao-aws zhichao-aws left a comment

Choose a reason for hiding this comment

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

lgtm

excludeTestsMatching "org.opensearch.neuralsearch.bwc.HybridSearchIT.*"
excludeTestsMatching "org.opensearch.neuralsearch.bwc.NeuralSparseSearchIT.*"
excludeTestsMatching "org.opensearch.neuralsearch.bwc.NeuralQueryEnricherProcessorIT.*"
excludeTestsMatching "org.opensearch.neuralsearch.bwc.restart.MultiModalSearchIT.*"
Copy link
Member

Choose a reason for hiding this comment

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

Can you also do the similar change rolling upgrade build.gradle

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no such thing for rolling upgrade build.gradle in main branch. I need to make such change when I backport it to 2.x

@heemin32 heemin32 merged commit c98b9c2 into opensearch-project:main Jan 15, 2025
39 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 15, 2025
heemin32 added a commit that referenced this pull request Jan 15, 2025
heemin32 added a commit that referenced this pull request Jan 15, 2025
heemin32 added a commit that referenced this pull request Jan 15, 2025
…1107)

Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit c98b9c2)

Co-authored-by: Heemin Kim <[email protected]>
heemin32 added a commit to heemin32/neural-search that referenced this pull request Jan 15, 2025
heemin32 added a commit to heemin32/neural-search that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch Refactoring Improve the design, structure, and implementation while preserving its functionality skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants