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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Make generic test source available #2336

Closed
Martin7-1 opened this issue Dec 26, 2024 · 2 comments · Fixed by langchain4j/langchain4j-spring#101 or #2337
Closed

[FEATURE] Make generic test source available #2336

Martin7-1 opened this issue Dec 26, 2024 · 2 comments · Fixed by langchain4j/langchain4j-spring#101 or #2337
Labels
enhancement New feature or request Testing

Comments

@Martin7-1
Copy link
Contributor

Is your feature request related to a problem? Please describe.

EmbeddingStore's IT in community repo may extend EmbeddingStoreIT, EmbeddingStoreWithFilteringIT, etc. Main repo should expose test source for better debug functionality.

Describe the solution you'd like

Adding plugin in langchain4j-core and langchain4j-spring-boot-tests as following:

            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-source-plugin</artifactId>
                <version>3.3.1</version>
                <executions>
                    <execution>
                        <id>attach-sources</id>
                        <goals>
                            <goal>jar-no-fork</goal>
                            <!-- For getting test source -->
                            <goal>test-jar-no-fork</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>

Describe alternatives you've considered

No

Additional context

No
@Martin7-1 Martin7-1 added the enhancement New feature or request label Dec 26, 2024
This was referenced Dec 26, 2024
Merged
Merged
@langchain4j
Copy link
Owner

Hmm, do you know why maven-source-plugin is not inherited from langchain4j-parent to langchain4j-core?

@Martin7-1
Copy link
Contributor Author

Martin7-1 commented Jan 2, 2025 •
edited
Loading

Hmm, do you know why maven-source-plugin is not inherited from langchain4j-parent to langchain4j-core?

The plugin need to be declared in <pluginManagement> so that it could be re-declared in sub-modules and for custom settings. (e.g. langchain4j-core need to set test-jar-no-fork, so it can redelcare it and add custom setting since maven-source-plugin is declared in <pluginManagement> in #2337).

As for plugins which are declared in <plugins> in langchain4j-parent, they are inherited directly to sub-modules, but they can't be redeclared directly in sub-modules.

Maven's official document said:

Important Note: Always define the version of each plugin used to guarantee build reproducibility. A good practice is to specify each build plugin's version in a element. Often the element is found in the parent POM. For reporting plugins, specify each version in the element (and in the element too).

So, the solution is: declare maven-source-plugin in both <pluginManagement> and <plugins> in langchain4j-parent. (This is actually the implementation right now, implemented in #2337 ). So the sub-modules can direct inherit from langchain4j-parent (this way they do not need to redeclare it) or they can redeclare it for custom settings.

Is this explanation solve your problem?

langchain4j pushed a commit to langchain4j/langchain4j-spring that referenced this issue Jan 3, 2025
@Martin7-1
Elasticsearch / Redis / Milvus: Remove lombok and small update for IT (…
c4fccf5 
…#101)

<!--
Thank you so much for your contribution!

Please fill in all the sections below.
Please open the PR as a draft initially. Once it is reviewed and
approved, we will ask you to add documentation and examples.
Please note that PRs with breaking changes or without tests will be
rejected.

Please note that PRs will be reviewed based on the priority of the
issues they address.
We ask for your patience. We are doing our best to review your PR as
quickly as possible.
Please refrain from pinging and asking when it will be reviewed. Thank
you for understanding!
-->

## Issue
<!-- Please specify the ID of the issue this PR is addressing. For
example: "Closes #1234" or "Fixes #1234" -->
Closes
[langchain4j#2336](langchain4j/langchain4j#2336)

## Change
<!-- Please describe the changes you made. -->

1. Remove `lombok` in Elasticsearch / Redis / Milvus
2. Configure `maven-source-plugin:test-jar-no-fork` for
`langchain4j-spring-boot-tests`, in order to see test source in
community repo.
3. Redis: update `prefix` parameter introducing in [this
PR](langchain4j/langchain4j#1981)

## General checklist
<!-- Please double-check the following points and mark them like this:
[X] -->
- [x] There are no breaking changes
- [ ] I have added unit and/or integration tests for my change
- [ ] The tests cover both positive and negative cases
- [x] I have manually run all the unit and integration tests in the
module I have added/changed, and they are all green
<!-- Before adding documentation and example(s) (below), please wait
until the PR is reviewed and approved. -->
- [ ] I have added/updated the
[documentation](https://github.com/langchain4j/langchain4j/tree/main/docs/docs)
- [ ] I have added an example in the [examples
repo](https://github.com/langchain4j/langchain4j-examples) (only for
"big" features)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request Testing
Projects
None yet
Milestone
No milestone
2 participants
@Martin7-1 @langchain4j