Skip to main content

Please review my fix for CR 6681630 "Convert Report doesn't provide ..."

7 replies [Last post]
sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

Plis reaview my fix for this issue.

diffs are:
https://jtharness.dev.java.net/source/browse/jtharness?rev=855&view=rev

Some notes:

"The problem is that Report Converter tool uses a different way to instantiate ReportEnvironment that regular Create Report tool.

@see com.sun.javatest.mrep.ReportTool#startMerge():
...
if (merger.merge(in,xmlOut, resolver)) {
CustomReport.ReportEnviroment re = new CustomReport.ReportEnviroment(xmlOut, in);
for (int i = 0; i < customReports.length; i++) {
...
CustomReport cr = customReports[i];
cr.setEnviroment(re);
cr.createReport(new File(out, cr.getReportId()));
}
...

As you can see the CustomReport.ReportEnviroment is created with the (File, File[]) constructor. And it's instance variable ReportEnviroment#ip (InterviewParameters) is not initialized."

"Problem is that the report converter is not associated with a particular instance of the test suite or work dir. So it is correct to return null - there is no associated interview when doing a conversion."

"I'm sure there MUST be an connection between the TestSuite and ReportConverter Tool - some kind of the shared context. Just because ReportConverter Tool lists "custom reports" that can be only taken from TestSuite's context (com.sun.javatest.exec.ContextManager.getCustomReports()). So - if there is a connection with Context - there should be also a connection with the TestSuite (because custom ContextManager is defined in the testsuite.jtt/"tmcontext")."

My note:

there exist connection between ReportConverter tool and ExecTool tool. More precisely, between report converter and all opened exec tools. Their intersection is in com.sun.javatest.mrep.OptionsPane.getCustomReports() method. This method invokes at report converter start up, and this is the place where report converter collects all custom reports from all open exec tool instances.

Solution to this bug is to create correct CustomReport.ReportEnviroment instances (as we have access to particular exec tool, its interview, context manager, custom report and all other things, we may be interested in) inside this method and setup them to respective custom reports.

Then in report generation period we should just set additional parameters(file names) to custom report enviroments, not to create them

Thanks,
Sergey Borodin

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

Suggested fix solves this particular issue - after it's integration each custom report inside merger tool will have properly initialized ReportEnviroment object.

But more general problem - duplication of custom reports, if 2 or more instances of the same test suite opened, is not solved. The main problem here is that custom reports use their ID as directory names, where to store results, thus reports with the same ID will overwrite each other.

From other point of view, it is not subject of 6681630, and, from my opinion, should be formalized as separate issue.

Thanks,
Sergey

kromanovs
Offline
Joined: 2007-11-14
Points: 0

I understand the problem with potentially duplicating reports. I agree this is not a part of this particular request.

In general I think that it could be a possible solution if Merge Report Tool would just alter ID and Displayable Name if it encounter "duplicate" reports from different ExecTools. Need to think about it.

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

It seems that java.net didn't accept my reply to this. :( I'll have to retype it.

In summary though, I don't think the implemenation is correct. It assumes a single exec tool that is associated with a report type. This may be true for some products, but for those which allow you to open a test suite multiple times, it would not work as desired.

I suggest that a way to do this should be found which forces the user to tell which workdir to use for context. But logically speaking, I'm really not sure that a merge operation of this type should be using information which is not in the XML content.

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

This fix was requested because someone wanted interview settings when making a report in the report converter tool. I believe a correct solution is to tell them that the their custom GUI for this report type will have to ask the user to supply a JTI file (or similar). Doing things in the report converter is independent of a work dir, so no particular should be "assumed" or "searched for".

So unless you have a different technical solution, I think the logical thing is for the report GUI to ask the user for any information it needs. Further, the requestor wanted info from getIp() - I don't think the harness can or should supply this info while in the report converter context. This is why the ability to collect input from the user is provided in custom reports - they will need to ask the user.

Additionally, the requestor will probably ask - so how do I know if I need to ask the user? Right now, the custom report API is re-used both inside and outside the Exec Tool context. Since the environments are different though, the API for a custom report will be slightly different for the two. In particular, an API designed for the Report Converter would _not_ have the method getIp(). Workaround could be used for now, but the correct solution may be to define a different interface for a Report Converter custom report.

kromanovs
Offline
Joined: 2007-11-14
Points: 0

I agree that Report Converter tool is somewhere different from the CreateReport that is directly coupled with respective Exec Tool.
ReportConverter stays more or less independent and it assumes reports will be created based from a source XML file(s).

However we should also admit that once Report Converter opens is DOES show available custom reports. As far as I can see it's implemented in the com.sun.javatest.mrep.OptionsPane.getCustomReports(JPanel) method:
...
Tool[] tools = desktop.getTools();
for (int i = 0; i < tools.length; i++) {
if (tools[i] instanceof ExecTool) {
ExecTool tool = (ExecTool) tools[i];
ContextManager cm = tool.getContextManager();
customReportsList.addAll(Arrays.asList(cm.getCustomReports()));
...

As you can see ReportConverter iterates over currently opened ExecTools and gathers instances of CustomReport from each one. This means that EACH of customReport instances is INDEED associated with the ExecTool - and thus with the TestSuite and possible with the WorkingDirectory and Interview.

So, from my point of view ReportConverter is really a separate tool, while CustomReports, hosted inside HAVE associations with particular ExecTools - even if there are multiple of them are opened in the Harness.

Basing on above I think Sergey did a good fix.

sergey_borodin
Offline
Joined: 2006-10-20
Points: 0

My opinion is that the best solution will be to remove custom report generation from report converter, and to insert feature of custom reports creation from existing xml source inside exec tool.

But I think we have no ability to implement this inside 4.1.4 timeframe.

--sergey

bkurotsu
Offline
Joined: 2004-12-13
Points: 0

People have told me that it's important to have this working. So I approve your change, but would like you to make a note in the javadoc that when using in the report converter, it searches for an interview in exec tool and returns it. If the user does not have an open an exec tool, it returns null?

And we should file an issue in Issue Tracker - "Provide way to get test suite context for custom report in report converter"