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.

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

Shrinkdes- 23 Web app descriptor tests fail because of carriage return #7

Merged
1 commit merged into from
Nov 23, 2010
Merged

Shrinkdes- 23 Web app descriptor tests fail because of carriage return #7

1 commit merged into from
Nov 23, 2010

Conversation

andygibson
Copy link
Contributor

This patch fixes the tests for the web app descriptors by stripping the carriage returns from the web-xxxx.xml resource files that are used to provide the expected results in the tests. The files look identical, but aren't due to carriage returns. This patch strips out the carriage returns so the tests can run correctly.

@ALRubinger
Copy link
Member

Have a look at this handling:

ALRubinger@0121821

It'd be nice to reply on platform default encoding rather than manually hacking the carriage returns. This approach using a Reader works for you?

@andygibson
Copy link
Contributor Author

Yours is a much cleaner and robust solution, but the method you wrote to get the resource file as a string doesn't fix the problem and the test still fails. I tweaked your version to use a BufferedReader and to read in line by line, and then build the content from those lines. I have tested it in Windows and it works.
Unfortunately, I messed up my original fork, oddly enough by adding carriage returns to the source which means it looks like every line has changed even though it hasn't, and I'm still trying to get the hang of Git. So, I re-forked it as SHRINKDESC-23b which contains the code that works without all the CRLF issues in the source, and did a pull request from there. Sorry about that,

@ALRubinger
Copy link
Member

Hard to sort through the real changes for this commit. :)

Recommend making a branch off the current master, and applying only the changes needed in an isolated commit.

Don't get discouraged; keep up the great work and contribution. Before long the Git/patch submission process will be second nature.

…ows.

Fixed as per ALRs suggestion that readers be used to read in the file representing the expected test results thus letting them handle the carriage returns as needed on the platform.
@andygibson
Copy link
Contributor Author

OK, I think I have it now. I deleted the my old fork, created a new one and re-applied the changes correctly. I think that even though the fork was deleted and re-created, the original pull request from it is still active and valid. So, I hope you can extract the changes from this pull request. If not, just let me know,
Cheers,
Andy Gibson

@andygibson
Copy link
Contributor Author

  1. The descriptor exporter builds the exported String line by line with a \n between each line. This method is just trying to build the expected result the same way. If you read the characters in, it always includes the carriage return so doing it line by line removes the issue of newline and carriage returns and lets us construct the result in the same way the Descriptor exporter does it.
  2. That is meant to be a StringBuilder which is what I always use, I'm breaking in a new laptop on a strange OS and a strange keyboard. It's not the first time I've selected the String buffer from auto completion by mistake. I'll change this over.

@ALRubinger
Copy link
Member

  1. Ah cool. I wonder if we should be opening another issue to configure the carriage return/linefeed type used in the generated XML. I think good options would be for carriage return, linefeed+return, and system default (System.getProperty("line.separator").
  2. Word.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants