You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "TestBoost (via GitHub)" <gi...@apache.org> on 2023/12/09 23:52:49 UTC
[PR] only extract the ClassPathXmlApplicationContext once [camel]
TestBoost opened a new pull request, #12386:
URL: https://github.com/apache/camel/pull/12386
# Description
<!--
- Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
-->
This pull request is to make sure only extract the `ctx` once from the xml file `org/apache/camel/spring/xml/handler/ErrorHandlerDefinitionParser.xml`. There are four tests in this test class. Every test tries to call `getBean()` from ctx to get an instance with its name and type. However, `getBean()` is not changing contents in the `ctx`. In other words, given that all tests do not try to change the contents of this `ctx` context and they are just trying to read from it, we can just create the `ctx` once to speed up the this test class.
When run on our own machine, the test runtime can jump from `4.873 s` to `3.2581 s`.
# Target
- [ ] 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
- [ ] 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).
<!--
# *Note*: trivial changes like, typos, minor documentation fixes and other small items do not require a JIRA issue. In this case your pull request should address just this issue, without pulling in other changes.
-->
# Apache Camel coding standards and style
- [ ] I checked that each commit in the pull request has a meaningful subject line and body.
<!--
If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
-->
- [ ] I have run `mvn clean install -DskipTests` locally and I have committed all auto-generated changes
<!--
You can run the aforementioned command in your module so that the build auto-formats your code. This will also be verified as part of the checks and your PR may be rejected if if there are uncommited changes after running `mvn clean install -DskipTests`.
You can learn more about the contribution guidelines at https://github.com/apache/camel/blob/main/CONTRIBUTING.md
-->
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "klease (via GitHub)" <gi...@apache.org>.
klease commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1856179600
Most of the tests in camel-spring-xml extend `org.apache.camel.spring.SpringTestSupport`. That also provides the method `getMandatoryBean(Class<T> type, String name)` which would allow to simplify the code in your test.
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "TestBoost (via GitHub)" <gi...@apache.org>.
TestBoost commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1852821895
Thank you so much. I try to search in the test codes in camel-spring-xml and only find this test look like this as of now. However, there are indeed some tests like `ParentContextRegistryTest` (extends SpringTestSupport) and `FindSingleByTypeTest` (extends ContextTestSupport). For these two tests, maybe it's also better to choose `CamelSpringTestSupport` instead if there are no dependencies issues.
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "TestBoost (via GitHub)" <gi...@apache.org>.
TestBoost commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1849426344
Thank you for the response. And I indeed find in the `CamelSpringTestSupport`, there is a place to only read the `context` once under certain conditions. I just refactored as you said but I am not sure if it looks OK.
Thanks a lot
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1849433564
Oh we cannot depend on `camel-test-spring-junit5` as its a chicken and egg situation. As camel-spring-xml is a dependency on the test module, so you end up with circular dependency.
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus closed pull request #12386: only extract the ClassPathXmlApplicationContext once
URL: https://github.com/apache/camel/pull/12386
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "TestBoost (via GitHub)" <gi...@apache.org>.
TestBoost commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1857601821
Thank you so much!
I tried to extend `org.apache.camel.spring.SpringTestSupport` instead but I found `getMandatoryBean(Class<T> type, String name)` actually calls `applicationContext.getBean(name, type)`. My original goal was to create `ClassPathXmlApplicationContext` only once like `isCreateCamelContextPerClass()` in the `CamelSpringTestSupport` class. `CamelSpringTestSupport` extends `CamelTestSupport` and it tries to check if the context should be only created once.
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1849367972
Thanks, the test should really extend CamelSpringTestSupport and be something like
```
public class ErrorHandlerDefinitionParserTest extends SpringTestSupport {
@Override
protected AbstractXmlApplicationContext createApplicationContext() {
return new ClassPathXmlApplicationContext("org/apache/camel/spring/xml/handler/ErrorHandlerDefinitionParser.xml");
}
```
And `ctx` should then be `applicationContext`.
Can you try to change the PR to do that, thanks.
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "TestBoost (via GitHub)" <gi...@apache.org>.
TestBoost commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1849455576
Oh, I see and I did not notice this. So do we just extend `SpringTestSupport` rather than `CamelSpringTestSupport`? And it seems `SpringTestSupport` does not have the same way to check `isCreateCamelContextPerClass()`.
Thank you very much!
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1848784775
: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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1852358361
Did you notice that this was the only test that did this, or did you find others in camel-spring-xml
--
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] only extract the ClassPathXmlApplicationContext once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #12386:
URL: https://github.com/apache/camel/pull/12386#issuecomment-1935875115
Sorry closing old PRs - the camel-spring-xml should ideally be built before the test modules. And its fine for us as is.
--
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