sshaw / ddex

DDEX metadata serialization for Ruby
https://metadatagui.com
52 stars 41 forks source link

Added ERN 3.8.1 #9

Closed benalavi closed 8 years ago

sshaw commented 8 years ago

Hi, thanks so much for this.

The added spec (DDEX::ERN ERN 3.8.1 metadata serialization given instance1.xml serializes the metadata) is failing. Looks like it may be due to various elements in spec/fixtures/ern/381/instance1.xml being commented out. Was this intentional?

Would it also be possible to add 3.8 support? It looks like there's just one difference between this and 3.8.

benalavi commented 8 years ago

I could have sworn everything was passing before I pushed but I just checked and failing here as well. I actually just copied the oxygen sample configuration from 37.xml and changed the version numbers, so whatever is in that instance1.xml is just what oxygen generated. Admittedly I don't know enough about the full DDEX spec or how this XML sample generator works to know if that is a bad idea or not :)

Will add 3.8 and get the spec passing.

Also thanks for the link I couldn't find the changelog when I looked!

sshaw commented 8 years ago

I actually just copied the oxygen sample configuration from 37.xml and changed the version numbers, so whatever is in that instance1.xml is just what oxygen generated.

:scream_cat:

Admittedly I don't know enough about the full DDEX spec or how this XML sample generator works to know if that is a bad idea or not :)

Yeah would be better to generate something that had 3.8 elements in it to truly flush out the code generation.

Also thanks for the link I couldn't find the changelog when I looked!

Yes, the DDEX site is just like the spec...

benalavi commented 8 years ago

Yeah would be better to generate something that had 3.8 elements in it to truly flush out the code generation.

Haha yes after a little more digging I guess I should actually generate a test that tests something. Will put something together for 38 and 381.

benalavi commented 8 years ago

Added 3.8 and specs are passing with updated instances.

The large number of XML comments was because of the generator option to generate all results of choice commented. I ended up using that to generate a few instances and then compared the results with the relevant info in the changelog (of which there is little...) to try to ensure that newly added features were actually present in the XML sample. For instance 3.8 has an instant gratification deal w/o a pre-order period, artists w/ nationality, etc... 3.8.1 uses datetimes in a deal that don't appear in ERN 3.8. So if I understand correctly the specs should now be testing that the changes in those ERN versions are actually reflected in the generated code.

I believe CI may be failing for some rubies because of: https://github.com/bundler/bundler/issues/3558 (old version of bundler installed by Travis CI)

sshaw commented 8 years ago

Hey great thanks!

Any suggestions on how to make the generation and testing process easier? Or is it already super easy? :sunglasses:

benalavi commented 8 years ago

No problem!

The contributing guide pretty much covered all of it. What I did in case it is helpful in the future:

Most of the Oxygen sample configuration is setting types properly for various elements/attributes (for instance telling Oxygen to stick to true/false for booleans instead of randomly using 0/1 as well). Without setting those Oxygen will generate random things that are able to be read by the generated code but come out differently when serialized back to XML (0 becomes false for instance) causing the test to fail. A global setting for those would cut out a lot of of the sample configuration.

The link you posted to the release notes was the only way I could find what new changes actually need to be tested: https://kb.ddex.net/display/ERN38/Annex+B+%28informative%29+Release+Notes. Diff-ing the XSDs was not very helpful because of formatting differences :/ but maybe there is a better tool to detect schema changes between XSD versions.

From the release notes you can browse the relevant XSD (I used Oxygen to do that) and find the related schema changes.

Then check that the relevant schema changes appear in the instance.xml generated by Oxygen and add them if not. It was useful here to tell Oxygen to generate commented out sections for all options so that there would be some sample of the XML even if none were randomly chosen.

If the new schema changes can be read and written by the generated code then that seems like a good indicator that the correct schema was used and the code was generated correctly.

sshaw commented 8 years ago

Thanks for the feedback.

Most of the Oxygen sample configuration is setting types properly for various elements/attributes...

Maybe a Rake task can help here. Something like:

~/code/ruby/ddex >rake oxygen[SomeElement,boolean] FILE=etc/oxygen/samples/ern/381.xml

Diff-ing the XSDs was not very helpful because of formatting differences :/

What I usually do is this:

~/code/ruby/ddex >xmllint --format etc/schemas/ern/381/release-notification.xsd > /tmp/381.xsd
~/code/ruby/ddex >xmllint --format etc/schemas/ern/38/release-notification.xsd > /tmp/38.xsd
~/code/ruby/ddex >diff /tmp/381.xsd /tmp/38.xsd

Though it's probably a good idea to just add a Rake task do to this too. Can probably just call this method, which will produce a cleaner diff than the above.