You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "cziesman (via GitHub)" <gi...@apache.org> on 2024/03/12 01:46:03 UTC
[PR] Camel 20341 [camel]
cziesman opened a new pull request, #13447:
URL: https://github.com/apache/camel/pull/13447
# Description
Refactored `ContextTestSupport` to work like `CamelTestSupport` in order to fix brittle unit tests with timing issues. Updated affected classes and fixed the few remaining broken unit tests that extend `ContextTestSupport`.
# Target
- [ X] I checked that the commit is targeting the correct branch (note that Camel 3 uses `camel-3.x`, whereas Camel 4 uses the `main` branch)
# Tracking
- [ X] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
# Apache Camel coding standards and style
- [ X] I checked that each commit in the pull request has a meaningful subject line and body.
- [ ] I have run `mvn clean install -DskipTests` locally and I have committed all auto-generated changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1523619877
##########
components/camel-lucene/src/main/docs/lucene-component.adoc:
##########
@@ -108,7 +108,7 @@ RouteBuilder builder = new RouteBuilder() {
[source,java]
-----------------------------------------------------------------
-CamelContext context = new DefaultCamelContext(createRegistry());
+CamelContext context = new DefaultCamelContext(createCamelRegistry());
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521770015
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
Is there a process in place when reviewing code submissions to make sure that manual tests are named appropriately such that they do not run as part of the unit tests?
These changes are technically outside the scope of this PR, but these manual tests were breaking the unit tests, so they needed to be renamed to match their purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521755250
##########
core/camel-core/src/test/java/org/apache/camel/ContextTestSupport.java:
##########
@@ -338,53 +919,86 @@ protected void assertPredicate(String languageName, String expressionText, Excha
Predicate predicate = language.createPredicate(expressionText);
assertNotNull(predicate, "No Predicate could be created for text: " + expressionText + " language: " + language);
- assertPredicate(predicate, exchange, expected);
+ TestSupport.assertPredicate(predicate, exchange, expected);
}
/**
* Asserts that the language name can be resolved
*/
protected Language assertResolveLanguage(String languageName) {
Language language = context.resolveLanguage(languageName);
- assertNotNull(language, "No language found for name: " + languageName);
+ assertNotNull(language, "Nog language found for name: " + languageName);
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521141356
##########
core/camel-core/src/test/java/org/apache/camel/ContextTestSupport.java:
##########
@@ -338,53 +919,86 @@ protected void assertPredicate(String languageName, String expressionText, Excha
Predicate predicate = language.createPredicate(expressionText);
assertNotNull(predicate, "No Predicate could be created for text: " + expressionText + " language: " + language);
- assertPredicate(predicate, exchange, expected);
+ TestSupport.assertPredicate(predicate, exchange, expected);
}
/**
* Asserts that the language name can be resolved
*/
protected Language assertResolveLanguage(String languageName) {
Language language = context.resolveLanguage(languageName);
- assertNotNull(language, "No language found for name: " + languageName);
+ assertNotNull(language, "Nog language found for name: " + languageName);
Review Comment:
Typo here.
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
These are other changes in components (`camel-qdrant` and `camel-lucene`) are outside the context of this PR.
##########
core/camel-core/src/test/java/org/apache/camel/ContextTestSupport.java:
##########
@@ -258,7 +773,55 @@ protected <T extends Endpoint> T resolveMandatoryEndpoint(String uri, Class<T> e
* @return the mandatory mock endpoint or an exception is thrown if it could not be resolved
*/
protected MockEndpoint getMockEndpoint(String uri) {
- return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ return getMockEndpoint(uri, true);
+ }
+
+ /**
+ * Resolves the {@link MockEndpoint} using a URI of the form <code>mock:someName</code>, optionally creating it if
+ * it does not exist. This implementation will lookup existing mock endpoints and match on the mock queue name, eg
+ * mock:foo and mock:foo?retainFirst=5 would match as the queue name is foo.
+ *
+ * @param uri the URI which typically starts with "mock:" and has some name
+ * @param create whether or not to allow the endpoint to be created if it doesn't exist
+ * @return the mock endpoint or an {@link NoSuchEndpointException} is thrown if it could not
+ * be resolved
+ * @throws NoSuchEndpointException is the mock endpoint does not exist
+ */
+ protected MockEndpoint getMockEndpoint(String uri, boolean create) throws NoSuchEndpointException {
+ // look for existing mock endpoints that have the same queue name, and
+ // to
+ // do that we need to normalize uri and strip out query parameters and
+ // whatnot
+ String n;
+ try {
+ n = URISupport.normalizeUri(uri);
+ } catch (URISyntaxException e) {
+ throw RuntimeCamelException.wrapRuntimeException(e);
+ }
+ // strip query
+ final String target = StringHelper.before(n, "?", n);
+
+ // lookup endpoints in registry and try to find it
+ MockEndpoint found = (MockEndpoint) context.getEndpointRegistry().values().stream()
+ .filter(e -> e instanceof MockEndpoint).filter(e -> {
+ String t = e.getEndpointUri();
+ // strip query
+ int idx2 = t.indexOf('?');
+ if (idx2 != -1) {
+ t = t.substring(0, idx2);
+ }
+ return t.equals(target);
+ }).findFirst().orElse(null);
+
+ if (found != null) {
+ return found;
+ }
+
+ if (create) {
+ return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ } else {
+ throw new NoSuchEndpointException(String.format("MockEndpoint %s does not exist.", uri));
+ }
Review Comment:
This one seems similar enough to what we have in the `CamelTestSupport` that I wonder if it should be de-duplicated in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521831278
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
1. Why would these tests break anything here?
2. These tests are not manual tests
So, leave them outside the scope of this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1522046054
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
My apologies for renaming the Quadrant test classes in error. When I started working on these fixes, the Quadrant tests were failing due to Docker errors, so I assumed that they needed to be run manually. The tests are no longer failing, so I have reverted the class names. Thanks for catching this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521889388
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
The Quadrant tests break the unit test build because they assume the presence of Docker without using TestContainers. Unit tests by definition should be self contained and not depend on external resources. I can revert the class names but there are many tests within Camel that depend on Docker that use the `*ManualIT.java` naming convention.
Perhaps there should be a Camel standard that (1) Docker is only used with TestContainers, or that (2) tests that depend on Docker should be named appropriately and run manually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521838890
##########
core/camel-core/src/test/java/org/apache/camel/ContextTestSupport.java:
##########
@@ -258,7 +773,55 @@ protected <T extends Endpoint> T resolveMandatoryEndpoint(String uri, Class<T> e
* @return the mandatory mock endpoint or an exception is thrown if it could not be resolved
*/
protected MockEndpoint getMockEndpoint(String uri) {
- return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ return getMockEndpoint(uri, true);
+ }
+
+ /**
+ * Resolves the {@link MockEndpoint} using a URI of the form <code>mock:someName</code>, optionally creating it if
+ * it does not exist. This implementation will lookup existing mock endpoints and match on the mock queue name, eg
+ * mock:foo and mock:foo?retainFirst=5 would match as the queue name is foo.
+ *
+ * @param uri the URI which typically starts with "mock:" and has some name
+ * @param create whether or not to allow the endpoint to be created if it doesn't exist
+ * @return the mock endpoint or an {@link NoSuchEndpointException} is thrown if it could not
+ * be resolved
+ * @throws NoSuchEndpointException is the mock endpoint does not exist
+ */
+ protected MockEndpoint getMockEndpoint(String uri, boolean create) throws NoSuchEndpointException {
+ // look for existing mock endpoints that have the same queue name, and
+ // to
+ // do that we need to normalize uri and strip out query parameters and
+ // whatnot
+ String n;
+ try {
+ n = URISupport.normalizeUri(uri);
+ } catch (URISyntaxException e) {
+ throw RuntimeCamelException.wrapRuntimeException(e);
+ }
+ // strip query
+ final String target = StringHelper.before(n, "?", n);
+
+ // lookup endpoints in registry and try to find it
+ MockEndpoint found = (MockEndpoint) context.getEndpointRegistry().values().stream()
+ .filter(e -> e instanceof MockEndpoint).filter(e -> {
+ String t = e.getEndpointUri();
+ // strip query
+ int idx2 = t.indexOf('?');
+ if (idx2 != -1) {
+ t = t.substring(0, idx2);
+ }
+ return t.equals(target);
+ }).findFirst().orElse(null);
+
+ if (found != null) {
+ return found;
+ }
+
+ if (create) {
+ return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ } else {
+ throw new NoSuchEndpointException(String.format("MockEndpoint %s does not exist.", uri));
+ }
Review Comment:
It's not about the class. It's about the body of the method.
The duplication can be resolved by creating a new component and properly adjusting the code.
But, in retrospect, I don't think you need to do anything here. This type of fixes is better done by those of us with more experience on the code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1522793849
##########
components/camel-lucene/src/main/docs/lucene-component.adoc:
##########
@@ -108,7 +108,7 @@ RouteBuilder builder = new RouteBuilder() {
[source,java]
-----------------------------------------------------------------
-CamelContext context = new DefaultCamelContext(createRegistry());
+CamelContext context = new DefaultCamelContext(createCamelRegistry());
Review Comment:
This one should also not be changed. It's not part of the PR scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521889388
##########
components/camel-qdrant/src/test/java/org/apache/camel/component/qdrant/QdrantUpsertManualIT.java:
##########
@@ -31,7 +31,7 @@
import static io.qdrant.client.ValueFactory.value;
import static org.assertj.core.api.Assertions.assertThat;
-public class QdrantUpsertTest extends QdrantTestSupport {
+public class QdrantUpsertManualIT extends QdrantTestSupport {
Review Comment:
The Quadrant tests break the unit test build because they assume the presence of Docker without using TestContainers. Unit tests by definition should be self contained and not depend on external resources. I can revert the class names but there are many tests within Camel that depend on Docker that use the `*ManualIT.java` naming convention.
Perhaps there should be a Camel standard that (1) Docker is only used with TestContainers, or that (2) tests that depend on Docker should be named appropriately and run manually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13447:
URL: https://github.com/apache/camel/pull/13447#issuecomment-1989766013
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot.
* You can label PRs using `build-all`, `build-dependents`, `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the Summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
* :warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "cziesman (via GitHub)" <gi...@apache.org>.
cziesman commented on code in PR #13447:
URL: https://github.com/apache/camel/pull/13447#discussion_r1521762957
##########
core/camel-core/src/test/java/org/apache/camel/ContextTestSupport.java:
##########
@@ -258,7 +773,55 @@ protected <T extends Endpoint> T resolveMandatoryEndpoint(String uri, Class<T> e
* @return the mandatory mock endpoint or an exception is thrown if it could not be resolved
*/
protected MockEndpoint getMockEndpoint(String uri) {
- return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ return getMockEndpoint(uri, true);
+ }
+
+ /**
+ * Resolves the {@link MockEndpoint} using a URI of the form <code>mock:someName</code>, optionally creating it if
+ * it does not exist. This implementation will lookup existing mock endpoints and match on the mock queue name, eg
+ * mock:foo and mock:foo?retainFirst=5 would match as the queue name is foo.
+ *
+ * @param uri the URI which typically starts with "mock:" and has some name
+ * @param create whether or not to allow the endpoint to be created if it doesn't exist
+ * @return the mock endpoint or an {@link NoSuchEndpointException} is thrown if it could not
+ * be resolved
+ * @throws NoSuchEndpointException is the mock endpoint does not exist
+ */
+ protected MockEndpoint getMockEndpoint(String uri, boolean create) throws NoSuchEndpointException {
+ // look for existing mock endpoints that have the same queue name, and
+ // to
+ // do that we need to normalize uri and strip out query parameters and
+ // whatnot
+ String n;
+ try {
+ n = URISupport.normalizeUri(uri);
+ } catch (URISyntaxException e) {
+ throw RuntimeCamelException.wrapRuntimeException(e);
+ }
+ // strip query
+ final String target = StringHelper.before(n, "?", n);
+
+ // lookup endpoints in registry and try to find it
+ MockEndpoint found = (MockEndpoint) context.getEndpointRegistry().values().stream()
+ .filter(e -> e instanceof MockEndpoint).filter(e -> {
+ String t = e.getEndpointUri();
+ // strip query
+ int idx2 = t.indexOf('?');
+ if (idx2 != -1) {
+ t = t.substring(0, idx2);
+ }
+ return t.equals(target);
+ }).findFirst().orElse(null);
+
+ if (found != null) {
+ return found;
+ }
+
+ if (create) {
+ return resolveMandatoryEndpoint(uri, MockEndpoint.class);
+ } else {
+ throw new NoSuchEndpointException(String.format("MockEndpoint %s does not exist.", uri));
+ }
Review Comment:
Per my conversation with Claus, both classes are needed since `came-core` cannot use `camel-test` due to circular dependencies, and `ContextTestSupport` is not intended for use outside of `camel-core`'. As a result, `CamelContextSupport` and `ContextTestSupport` will contain duplicated code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Camel 20341 [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske merged PR #13447:
URL: https://github.com/apache/camel/pull/13447
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org