JsonExporter changes

[wborkofsky]

No description provided.

[bestchai]

Has all this new code been tested? I don't see any changes to tests as part of this pull request.

[ohmann]

First round of comments, sending because the code is getting a little messy for me to look through, so I'll wait for you to address this first round before I do a second round of comments. If you notice that one of my comments also applies to other areas of your code I didn't explicitly comment on, please make the relevant changes in those spots as well to prevent me from needing to make the same comments again next round. Also, please add tests as Ivan suggested (although I realize that my original JsonExporter code erroneously was untested as well). Thanks for doing this!

[ohmann]

I added this for debugging and illustration purposes only. Remove this line.

[ohmann]

Move this up to where displayablesList is declared and assigned.

[ohmann]

Why does the method name have New in its name? Unless this means something, remove it.

[ohmann]

Nodes -> nodes

[ohmann]

add -> Add

[ohmann]

Do the same thing I mentioned above for nodesIDMap.

[ohmann]

Just put evGraph.getNodes() directly within this foreach statement, since allNodes isn't used anywhere but here. Make the same change in all other methods that follow this same pattern.

[ohmann]

Same comment as for the allNodes foreach above.

[ohmann]

This means displayableValue will be set twice for every edge for Synoptic models and still twice for a few edges touching initial or terminal in Perfume models. Instead, just assign displayableValue once based on whether this condition is true (so basically do an if/else instead of just an if).

[ohmann]

Same comment as above global maps.

[ohmann]

Make the rest of these global maps private, and initialize all of them to null.

[ohmann]

ini -> init

[ohmann]

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()).

package synoptic.tests.units;

import java.util.Arrays;

import org.junit.Before;
import org.junit.Test;

import synoptic.main.AbstractMain;
import synoptic.main.PerfumeMain;
import synoptic.main.options.PerfumeOptions;
import synoptic.model.PartitionGraph;
import synoptic.model.export.DotExportFormatter;
import synoptic.model.export.JsonExporter;

/**
 * TODO
 */
public class JsonExporterTests {
    private PerfumeMain main;
    private PartitionGraph pGraph;

    @Before
    public void setUp() throws Exception {
        // Set up Perfume options
        PerfumeOptions perfOpts = new PerfumeOptions();
        perfOpts.regExps = Arrays.asList(
                new String[] { "(?<ip>.+), (?<TYPE>.+), (?<DTIME>.+)" });
        perfOpts.partitionRegExp = "\\k<ip>";
        perfOpts.logFilenames.add(
                "../traces/abstract/perfume-survey/browser-caching-traces.txt");

        // Create a Perfume instance
        AbstractMain.instance = null;
        main = new PerfumeMain(perfOpts.toAbstractOptions(),
                new DotExportFormatter());

        // Run Perfume
        pGraph = main.createInitialPartitionGraph();
        main.runSynoptic(pGraph);
    }

    /**
     * TODO
     */
    @Test
    public void renameMeTest() throws Exception {
        JsonExporter.exportJsonObject("/tmp/JsonExporterTest", pGraph);
    }
}

[ohmann]

hg revert this file

[ohmann]

hg revert this file

[ohmann]

In general (but almost always), global variables should be avoided when possible. Almost all of these should not be global and should be private, method-level variables. For example, main is only used within setUp(). The only reason a global variable should be used is if the state of an object needs to be shared between methods (not just the variable being re-initialized with the same name in different methods but actually the same object) and if there is no other convenient way to pass that object between the methods (e.g., parameters, return values). Please make these local variables instead of global.

[ohmann]

As we discussed over email, different methods of running these unit tests result in different behaviors when using relative paths like this one (the current directory when running the tests is either $repo/synoptic/synoptic or $repo/synoptic/synoptic/bin). Therefore, this test will fail when run in some ways and pass in others.

Add a static method somewhere (perhaps SynopticTest) that resolves paths for files used by unit tests. Make the method something like getTestPath(String path) that takes a relative path like this--but probably omit the ./--and checks if the file as written works. If not, it checks if ../$pathAsWritten works. If neither work, it throws an exception. Call that new method from here to get the path for this txt file.

[ohmann]

See comments. Also remove file abc

[ohmann]

Some of the commented-out test code below looks useful. You could verify that each of the expected displayable values (e.g., "cache-page [9,17]") is present, right? The latter could be done quickly by populating a set of all the displayable values in expected vs. in reality and then simply checking expectedSet.equals(actualSet) (but perhaps with better variable names). That lets you make java Set equality do all the work. You could do similar check for other lists that have meaningful string values, like invariantTypes and logStatements.

However, I totally understand your desire to avoid doing a very long and complex check that the entire graph of edges and nodes match up. :) I think my suggestions above hit a nice compromise, since they shouldn't be too difficult or long, but they still verify real expected values rather than only verifying that the produced JSON has the right lists that we expect and doesn't have nulls.

[ohmann]

There is redundancy here (the m.containsKey("id") check), plus you should fail immediately when you encounter a problem rather than using res. Basically:

if (m.containsKey("id")) {
    if (m.get("id") == null) {
        fail("Displayable contains null id");
    }
    idCount++;
}

Remove res entirely from this test and others, and change all of the conditionals to check this way. I'm not sure if you need to import anything or do the fail() call slightly differently, perhaps Assert.fail() instead, basically, use this.

[ohmann]

Nit: Capitalize to keep comments consistent with the rest of the codebase ("Original path passed in"). Same for other comments

[ohmann]

Ensure the path startsWith("../") or "..\" first. Also, there are more robust ways to do these kinds of path manipulations in general, like using Paths and Path, but I'm OK with these simple ways, since this is only used in our tests.

[ohmann]

@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!

[ohmann]

@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?"

[wborkofsky]

@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.