Open wborkofsky opened 7 years ago
@wborkofsky I apologize that I forgot about this pull request. Did your changes after my March 29 comments address all of my comments, i.e., am I up to bat on this? I saw that at least some of the comments were addressed, but I saw that you didn't remove the abc
file, and I didn't look closely enough to see if you did anything with my request to do a set comparison of some of the fields for actual vs. expected, e.g., expected edge displayables are {"cache-image", "retrieve-image"}, then ensure the actual JSON contains exactly those same edge displayables. (As I mentioned in the comment, it would be hard and annoying to ensure the expected/actual JSON exactly matches up because IDs will change, but this is sort of getting halfway there.)
If everything is addressed or you have responses to any comments you didn't address, please let me know, and I'll take another look!
@wborkofsky Just a quick ping on my question from above: "Did your changes after my March 29 comments address all of my comments, i.e., am I up to bat on this?"
@ohmann It appears that I did add in all the test code for set equality. For the file "abc", in my commit, it shows as all lines removed so I believe it should have been removed as well and it does not appear in the synoptic version on my computer.
You can manually run the whole Perfume algorithm to get a partition graph for your tests. Try this skeleton test class I made, and you can add your actual tests to it. Please don't actually use file i/o like my fake renameMeTest does. Change JsonExporter so that you can test it without actually outputting a file (just refactor it so that you aren't forced to generate the json and output a file, since this is currently all within the only entry point, exportJsonObject()).