UTF-8 encoding bug

[GoGoris]

Hello,

I found that there is a bug that makes the concordion test fail when there are any uncommon characters like é or è in the excel file. This is really an issue with French, which is one of the main languages in Belgium. When these characters appear, a white square is shown as expected character instead of the real character.

Because you project is open source, I could trace back the bug in ExcelClassPathSource on line 54, you forgot to specify the encoding there (so java will use the default encoding instead of UTF-8):

Faulty line: return new ByteArrayInputStream(resultString.getBytes()); Fix: return new ByteArrayInputStream(resultString.getBytes("UTF-8"));

After changing this, the test seems to work as expected.

Kind regards, GoGoris

[nigelcharman]

Thanks for the issue report.

It made me realise we have an issue with the new converter support in Concordion 2.0.0. I created a fix in concordion/concordion@28ba73e493282fa7152a32b2af24044ea41f3c3f. Interestingly, my JVM seemed to use UTF-8 by default, and I had to change to getBytes("US-ASCII") to get the test case to fail.

This doesn't fix your immediate issue, but prevents similiar issues with the Markdown format in 2.0, and will support UTF-8 when the Excel extension is ported over to use the 2.0 converters.

I'll leave it up to Rob to fix the immediate issue with the excel extension (shout out if you need a hand Rob!).

[GoGoris]

Unfortunately the default locale in Belgium is ISO-8859-1 on Windows. Looking forward to the fix or Concordion 2.0!

[robmoffat]

Just pushed it now, but there is one other change I need to incorporate before I can do a new release for concordion 2.0:

Nigel,

I’d like to also make the change to ConcordionExtender.withSpecificationType(String typeSuffix, SpecificationConverter converter) https://github.com/concordion/concordion/pull/148/files#diff-ef396bb3b91506ec6f51a7d6b89cb18dR202

but this is only in the 2.0-SNAPSHOT, and the build.gradle is using:

apply from: 'https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle' https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle'

which is using concordion 1.5.1.

Do you want to move this on?

thanks,

Rob

On 28 Jan 2016, at 10:29, Steven Goris notifications@github.com wrote:

Unfortunately the default locale in Belgium is ISO-8859-1 on Windows. Looking forward to the fix or Concordion 2.0!

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176112510.

[nigelcharman]

Rob

Ideally we'd have a release version that uses Concordion 1.5.1 with the bug fixes in, and a separate snapshot version that uses the new features in the 2.0.0-SNAPSHOT.

We're targetting end-Feb for the full 2.0 release at this stage.

As an example, Andrew Sumner just released a snapshot version of the logback extension. He overrode the repository and versions in his main build.gradle https://github.com/concordion/concordion-logback-extension/blob/master/build.gradle file.

If you're ok with this, I'm happy to help out with the release process, though I hope that we've made it easy (at least for the full release) after last time :)

Nigel.

On 28/01/16 23:44, Rob Moffat wrote:

Just pushed it now, but there is one other change I need to incorporate before I can do a new release for concordion 2.0:

Nigel,

I’d like to also make the change to ConcordionExtender.withSpecificationType(String typeSuffix, SpecificationConverter converter) https://github.com/concordion/concordion/pull/148/files#diff-ef396bb3b91506ec6f51a7d6b89cb18dR202

but this is only in the 2.0-SNAPSHOT, and the build.gradle is using:

apply from: 'https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle' https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle'

which is using concordion 1.5.1.

Do you want to move this on?

thanks,

Rob

On 28 Jan 2016, at 10:29, Steven Goris notifications@github.com wrote:

Unfortunately the default locale in Belgium is ISO-8859-1 on Windows. Looking forward to the fix or Concordion 2.0!

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176112510.

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176118699.

[robmoffat]

Ok, that’s broadly done. However there is one snag. Previously, because I was being passed a Resource, rather than an InputStream, I was able to use the name of the resource to name the HTML file. However now, in ExcelSpecificationConverter I have this:

@Override public InputStream convert(InputStream inputStream) throws IOException { // TODO: we need to somehow source the name of the test here. This was originally from the filename. String testName = "Unknown Test"; HTMLBuilder result = new HTMLBuilderImpl(); workbookStrategy.process(new WorkbookHelper(inputStream, testName), result); inputStream.close();

    return createInputStreamFromPage(result);
}

I have checked all the code in, all the test pass, but each HTML report says “Unknown Test” at the top of it. Which is a bit annoying. What do you suggest?

cheers,

Rob

On 28 Jan 2016, at 17:38, Nigel Charman notifications@github.com wrote:

Rob

Ideally we'd have a release version that uses Concordion 1.5.1 with the bug fixes in, and a separate snapshot version that uses the new features in the 2.0.0-SNAPSHOT.

We're targetting end-Feb for the full 2.0 release at this stage.

As an example, Andrew Sumner just released a snapshot version of the logback extension. He overrode the repository and versions in his main build.gradle https://github.com/concordion/concordion-logback-extension/blob/master/build.gradle file.

If you're ok with this, I'm happy to help out with the release process, though I hope that we've made it easy (at least for the full release) after last time :)

Nigel.

On 28/01/16 23:44, Rob Moffat wrote:

Just pushed it now, but there is one other change I need to incorporate before I can do a new release for concordion 2.0:

Nigel,

I’d like to also make the change to ConcordionExtender.withSpecificationType(String typeSuffix, SpecificationConverter converter) https://github.com/concordion/concordion/pull/148/files#diff-ef396bb3b91506ec6f51a7d6b89cb18dR202

but this is only in the 2.0-SNAPSHOT, and the build.gradle is using:

apply from: 'https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle' https://raw.githubusercontent.com/concordion/concordion-extension-build/master/extension-build.gradle'

which is using concordion 1.5.1.

Do you want to move this on?

thanks,

Rob

On 28 Jan 2016, at 10:29, Steven Goris notifications@github.com wrote:

Unfortunately the default locale in Belgium is ISO-8859-1 on Windows. Looking forward to the fix or Concordion 2.0!

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176112510.

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176118699.

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176297129.

[nigelcharman]

OK. For clarity, by "name the HTML file" do you mean creating a header/title inside the HTML? The HTML file itself should be created by the Concordion core code.

How about we add a String resourceName parameter to the SpecificationConverter interface?

[robmoffat]

yes, this would be the title of the HTML page. That should do it.

On 29 Jan 2016, at 18:22, Nigel Charman notifications@github.com wrote:

OK. For clarity, by "name the HTML file" do you mean creating a header/title inside the HTML? The HTML file itself should be created by the Concordion core code.

How about we add a String resourceName parameter to the SpecificationConverter interface?

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176895443.

[nigelcharman]

@robmoffat I've merged this change into Concordion core. It will be in the next snapshot (probably next week). If you want to try it in the meantime, you can publish to your maven local.

[robmoffat]

Hi Nigel,

I rebuilt your latest concordion snapshot locally and then tried to build my project using that as a dependency. It’s clearly going to work, but there are all sorts of issues with test jars and versioning so I’ll wait until you get a new snapshot release onto jfrog beforeI commit this, otherwise the build will be broken for anyone else.

Any idea when that might be?

cheers,

Rob

On 29 Jan 2016, at 19:53, Nigel Charman notifications@github.com wrote:

@robmoffat https://github.com/robmoffat I've merged this change into Concordion core. It will be in the next snapshot (probably next week). If you want to try it in the meantime, you can publish to your maven local https://github.com/concordion/concordion/blob/master/README.md#installing-a-jar-file-into-your-local-maven-repository.

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-176939357.

[nigelcharman]

Hi Rob

I'm planning to make a new snapshot later today.

Concordion 2.0 now has breaking changes to the Source and SpecificationLocator interfaces, so the current version of the Excel extension won't work with Concordion 2.0. Sorry about that - it's the only known extension we've broken!

What I have done is add a check for the current excel extension to Concordion core. If it finds the org.concordion.ext.excel.ExcelClassPathSource class on the classpath, it throws an exception with the message "The Concordion Excel Extension must be updated to the latest version to work with Concordion 2.0 or later.". Let me know if you have any concerns with this, or want the exception message changed. If you let me know the new version number, I can change from "the latest version" to whatever the new version number is.

cheers

[robmoffat]

I've just committed changes making HEAD work with 2.0-SNAPSHOT of Concordion.

I don't think I'll be able to do a release version of this because of the snapshot dependency. I could do a SNAPSHOT release, so perhaps I should release 2.0-SNAPSHOT of C-E-E?

[nigelcharman]

That would be great. We've made it 2.0.0-SNAPSHOT in Concordion (an extra ".0" to what you have suggested).

Hopefully you can publish with "./gradlew clean publishSnapshot". Let me know if any issues.

On 01/03/16 08:28, Rob Moffat wrote:

I've just committed changes making HEAD work with 2.0-SNAPSHOT of Concordion.

I don't think I'll be able to do a release version of this because of the snapshot dependency. I could do a SNAPSHOT release, so perhaps I should release 2.0-SNAPSHOT of C-E-E?

— Reply to this email directly or view it on GitHub https://github.com/concordion/concordion-excel-extension/issues/5#issuecomment-190346555.

[robmoffat]

Released as part of 2.0.0.

[GoGoris]

Sorry for the late response, we had some dependency conflicts with xmlbeans that prevented us from updating to Concordion 2.0 any earlier. I can confirm that this issue is solved with Concordion 2.0.