-
Notifications
You must be signed in to change notification settings - Fork 12
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
Ability to dump graph as a dot file #125
base: master
Are you sure you want to change the base?
Conversation
.options() | ||
//.withRemoteDebugging() | ||
//.withRemoteSurefireDebugging() |
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.
please, clean it
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.
Sure, you could do that too :P straight from GH
.withSystemProperties(systemPropertiesPairs) | ||
.withSystemProperties("graph.name", name.getMethodName()) | ||
.withSystemProperties("smart.testing.debug", "true") // This will only be propagated to surefire for "not_reusing_forks" option |
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.
wouldn't be good to have these properties stored as constants somewhere...?
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.
Sure, it's still WIP as it needs to be aligned with Debug Mode once we have it.
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.
then add the label :-P
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.
or read the description :P
Graph is dumped when debug mode is enabled (should be aligned with #91)
@@ -76,6 +76,10 @@ void buildTestDependencyGraph(Collection<File> testJavaFiles) { | |||
addToIndex(new JavaElement(javaClass), javaClass.getImports()); | |||
} | |||
} | |||
|
|||
if (Boolean.valueOf(System.getProperty("smart.testing.debug", "false"))) { |
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.
The default value is not necessary here - from the javadoc of Boolean.valueof
/**
* Returns a {@code Boolean} with a value represented by the
* specified string. The {@code Boolean} returned represents a
* true value if the string argument is not {@code null}
* and is equal, ignoring case, to the string {@code "true"}.
*
* @param s a string.
* @return the {@code Boolean} value represented by the string.
*/
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.
Good catch, but this will be gone as soon as we align with #91
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.
It's a bit more than that - see #113 (comment)
try { | ||
new DOTExporter<JavaElement, DefaultEdge>(component -> "\"" + component.getClassName() + "\"", | ||
null, null) | ||
.exportGraph(graph, new File(System.getProperty("java.io.tmpdir") + "/graph-" + graphName + ".dot")); |
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.
Log also the information that it has been exported and the location of the file
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.
wouldn't make sense to store the file inside of target/smart-testing
directory of the particular module/project? I guess, that this directory will contain all ST relevant information/files
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.
yeah - updated description with the tasks.
812738f
to
ae0edb3
Compare
Short description of what this resolves:
While working on issues with Affected strategy I wrote a simple piece of code dumping graph to a
dot
format. This can be later used either using regular textdiff
or make an image out of it. For examplewill produce graph in PNG format.
Changes proposed in this pull request:
GraphExporter
producing dot filesgraph.name
property or has random UUID insteadAs an example (to be removed before final merge) I used our forking test - this shows that we really need configuration file instead of relying on properties, as we are not propagating them to surefire forked processes and some tests don't dump their graphs.
Tasks
target/smart-testing
directory of the particular module/projectFixes #136