Re: [xwiki-devs] [xwiki-notifications] r21683 - platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-chart/src/test/java/org/xwiki/rendering/macro/chart
On Jun 29, 2009, at 11:21 AM, asiri (SVN) wrote:
Author: asiri Date: 2009-06-29 11:21:59 +0200 (Mon, 29 Jun 2009) New Revision: 21683
Modified: platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki- rendering-macro-chart/src/test/java/org/xwiki/rendering/macro/chart/ TestChartMacro.java Log: [misc] Moving the temporary chart image file generated during tests inside the target directory.
Modified: platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/ xwiki-rendering-macro-chart/src/test/java/org/xwiki/rendering/macro/ chart/TestChartMacro.java =================================================================== --- platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki- rendering-macro-chart/src/test/java/org/xwiki/rendering/macro/chart/ TestChartMacro.java 2009-06-29 09:01:23 UTC (rev 21682) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki- rendering-macro-chart/src/test/java/org/xwiki/rendering/macro/chart/ TestChartMacro.java 2009-06-29 09:21:59 UTC (rev 21683) @@ -38,6 +38,6 @@ */ protected File getChartImageFile(ChartMacroParameters parameters, String content) { - return new File("./chart.png"); + return new File("./target/chart.png");
This is very bad. You should never hardcode maven directories location. Thanks -Vincent
Hi,
- return new File("./chart.png");
+ return new File("./target/chart.png");
This is very bad. You should never hardcode maven directories location.
Shall I leave it the way it was? The problem with that was a chart image gets left behind after the tests, and since it is not inside the target/ directory it is not cleaned by maven. May be override the tearDown() method and cleanup the file? But I'm not sure if this is possible with the way rendering tests are done. Thanks. - Asiri
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Jun 29, 2009, at 11:35 AM, Asiri Rathnayake wrote:
Hi,
- return new File("./chart.png");
+ return new File("./target/chart.png");
This is very bad. You should never hardcode maven directories location.
Shall I leave it the way it was? The problem with that was a chart image gets left behind after the tests, and since it is not inside the target/ directory it is not cleaned by maven.
May be override the tearDown() method and cleanup the file? But I'm not sure if this is possible with the way rendering tests are done.
No, no. There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property. -Vincent
Hi,
May be override the tearDown() method and cleanup the file? But I'm
not sure if this is possible with the way rendering tests are done.
No, no.
There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property.
Fixed by using "java.io.tmpdir" system property. Thanks. - Asiri
-Vincent _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Jun 29, 2009, at 12:05 PM, Asiri Rathnayake wrote:
Hi,
May be override the tearDown() method and cleanup the file? But I'm
not sure if this is possible with the way rendering tests are done.
No, no.
There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property.
Fixed by using "java.io.tmpdir" system property.
This is bad. For several reasons: 1) This is a unit test and it doesn't need to prove that jfreechart works 2) Any application that creates files a bit everywhere are very bad. Even if you added a remove on shutdown hook it would still be bad since if the application was killed there would be leftovers. 3) It's easy for a unit test to control what happens and use mocks where need be. It also improves the code design in general. Thanks -Vincent
Hi, On Mon, Jun 29, 2009 at 3:52 PM, Vincent Massol <[email protected]> wrote:
On Jun 29, 2009, at 12:05 PM, Asiri Rathnayake wrote:
Hi,
May be override the tearDown() method and cleanup the file? But I'm
not sure if this is possible with the way rendering tests are done.
No, no.
There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property.
Fixed by using "java.io.tmpdir" system property.
This is bad. For several reasons:
1) This is a unit test and it doesn't need to prove that jfreechart works 2) Any application that creates files a bit everywhere are very bad. Even if you added a remove on shutdown hook it would still be bad since if the application was killed there would be leftovers. 3) It's easy for a unit test to control what happens and use mocks where need be. It also improves the code design in general.
It's not the ChartGenerator that generates the chart image file. ChartGenerator returns a byte[] which represents the chart. It's the ChartMacro which writes out the chart image file. I can avoid this problem by: 1. Declaring the chart image file generation method (generateChart) abstract in ChartMacro 2. Extending the ChartMacro with TestChartMacro (already done) 3. Overriding the generateChart method in ChartMacro and avoiding the chart image file being written to disk. This will work fine and no files will be written on disk. I refused to do this because we wil be marking generateChart() method as abstract only to make it easy for us to write test cases. WDYT? Thanks. - Asiri
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Mon, Jun 29, 2009 at 4:05 PM, Asiri Rathnayake < [email protected]> wrote:
Hi,
On Mon, Jun 29, 2009 at 3:52 PM, Vincent Massol <[email protected]>wrote:
On Jun 29, 2009, at 12:05 PM, Asiri Rathnayake wrote:
Hi,
May be override the tearDown() method and cleanup the file? But I'm
not sure if this is possible with the way rendering tests are done.
No, no.
There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property.
Fixed by using "java.io.tmpdir" system property.
This is bad. For several reasons:
1) This is a unit test and it doesn't need to prove that jfreechart works 2) Any application that creates files a bit everywhere are very bad. Even if you added a remove on shutdown hook it would still be bad since if the application was killed there would be leftovers. 3) It's easy for a unit test to control what happens and use mocks where need be. It also improves the code design in general.
It's not the ChartGenerator that generates the chart image file. ChartGenerator returns a byte[] which represents the chart. It's the ChartMacro which writes out the chart image file.
I can avoid this problem by:
1. Declaring the chart image file generation method (generateChart) abstract in ChartMacro
2. Extending the ChartMacro with TestChartMacro (already done)
3. Overriding the generateChart method in ChartMacro and avoiding the chart image file being written to disk.
This will work fine and no files will be written on disk. I refused to do this because we wil be marking generateChart() method as abstract only to make it easy for us to write test cases.
Oops, I meant protected :)
WDYT?
Thanks.
- Asiri
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Jun 29, 2009, at 12:35 PM, Asiri Rathnayake wrote:
Hi,
On Mon, Jun 29, 2009 at 3:52 PM, Vincent Massol <[email protected]> wrote:
On Jun 29, 2009, at 12:05 PM, Asiri Rathnayake wrote:
Hi,
May be override the tearDown() method and cleanup the file? But I'm
not sure if this is possible with the way rendering tests are done.
No, no.
There are 2 options: - Best: Do not create an image. Just verify that jfreechart is called and verify where it would create the image . To be done with some mocks. - Not as good: pass the location from the pom.xml. Actually it's already passed by surefire so you just need to retrieve it as a system property.
Fixed by using "java.io.tmpdir" system property.
This is bad. For several reasons:
1) This is a unit test and it doesn't need to prove that jfreechart works 2) Any application that creates files a bit everywhere are very bad. Even if you added a remove on shutdown hook it would still be bad since if the application was killed there would be leftovers. 3) It's easy for a unit test to control what happens and use mocks where need be. It also improves the code design in general.
It's not the ChartGenerator that generates the chart image file. ChartGenerator returns a byte[] which represents the chart. It's the ChartMacro which writes out the chart image file.
I can avoid this problem by:
1. Declaring the chart image file generation method (generateChart) abstract in ChartMacro
2. Extending the ChartMacro with TestChartMacro (already done)
3. Overriding the generateChart method in ChartMacro and avoiding the chart image file being written to disk.
This will work fine and no files will be written on disk. I refused to do this because we wil be marking generateChart() method as abstract only to make it easy for us to write test cases.
What you need is a class for storing images. A default implementation would store in a temporary location on the file system. Another implementation could store in memory or in a DB or... You'd then use a dynamic mock implementation for the test which does nothing. -Vincent
participants (2)
-
Asiri Rathnayake -
Vincent Massol