-
Notifications
You must be signed in to change notification settings - Fork 92
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
Added round-trip tests #49
Conversation
Instances of \r\n in the outlook vcards were being converted to \n on linux
Current coverage is
|
To elaborate on the second commit message, the outlook samples weren't working because, for instance,
in the original sample was being converted to
in the jcard, which was then being written as
when it was converted back to vcard format on Linux. This was causing the test to fail when it would read the second vcard in, convert it to jcard, and do an exact string comparison with the original jcard. |
File[] dir = input.listFiles(new ExtensionFilter(".vcf")); | ||
for (File vcf : dir) { | ||
String file = vcf.getName().toString(); | ||
if (file.startsWith("John_Doe_EVOLUTION") || file.startsWith("John_Doe_IPHONE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a short comment here explaining why this "if" statement is here?
Is it here because of the problem with escaped characters in extended properties not being unescaped? I guess I will fix this issue once I merge your pull request. Then, I can remove this if statement.
Thank you for noticing the issue with the newlines. I understand why you need to make copies of the vCard samples now. |
@Test | ||
public void convert_from_jcard() throws Throwable { | ||
File input = new File(JCardRoundTripTest.class.getResource("").toURI()); | ||
File[] dir = input.listFiles(new ExtensionFilter(".json")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could refactor lines 82 and 83 into a reusable method: getFilesWithExtension(".json")
Can now exclude test cases from specific checks, rather than excluding them from the entire test class.
I've refactored this to make it easier to add round-trip tests to other formats, and added tests to roundtrip from xcard to vcard v4.0 and from xcard to vcard v3.0 as proof of concept. I've added a note next to each excluded test case; apart from the character encoding issues, there are a couple of things causing some of the tests to fail that aren't necessarily bugs in the library; the vcards are semantically identical but not textually identical, e.g. I've added a way to compare the deserialized objects using |
There were a couple of issues relating to xml serialization: the outlook-2003 file was serialized with an
after converting to vcard and back, becomes
|
|
||
private Filter(String extension, String... excludes) { | ||
this.extension = "." + extension; | ||
if (excludes != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This null check is not necessary. If no vararg parameters are passed in, then the "excludes" variable will be an empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
"evolution", "mac_address_book", // string escape issue | ||
"iphone", "lotus_notes", // groups are reordered | ||
"outlook-2007", // label got lost | ||
"rfc6350", // tel uri got lost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tel uri got lost
Only vCard version 4.0 (and xCard) supports tel URIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment for this exclusion to clarify that.
Was from "empty <street/> tag added to home adr" now "newlines not preserved on linux"
@Test | ||
public void compare_vcard_3_to_xcard() throws Exception { | ||
convertAllFromVCard(VCardVersion.V3_0, false, true, | ||
"android", // <pref><integer>1</integer></pref> removed from the ÑÑÑÑÑÑÑÑÑÑÑÑ email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<pref><integer>1</integer></pref> removed from the ÑÑÑÑÑÑÑÑÑÑÑÑ email
vCard versions 2.1 and 3.0 do not have a PREF parameter. Instead, they have a TYPE=PREF parameter. ez-vcard converts between the two.
Closing due to age. Sorry this got neglected. I wasn't comfortable merging it due to the large number of complex changes it involved. |
Continuing the discussion from https://github.com/bgorven/ez-vcard/commit/15813248e5b833cefdc6fb53892a529ccf42a938#commitcomment-16689669
The vcards look different after converting them to jcard and then back. I did it this way initially just to see if doing an exact string comparison of the converted files would work.
One benefit of having output files in the repo can be that if the output format changes at any time, you can see that in your repo. But it would be just as easy to create the files in a temp dir, which would keep the repo a lot tidier.