Hi there.
This may seem like a radical idea, and probably my most blunt idea I've ever posted but please bear with me:
NI folks working on TestStand as software, I don't mean to be offensive. I understand being in the position of having to make incremental changes, eventually outgrowing the original design of the software. I'm just saying what I think needs to be said and is long overdue.
Anyone who has had to customize the NI Report Generator will know that it is a complete hodgepodge of incremental changes from over the years.
As a high level summary:
- Within NI_ReportGenerator.seq, all of the report types (ATML, XML, HTML, text, etc.) are handled in the same sequences
- Depending on the report type, different steps are executed via preconditions, Post Actions (essentially GOTOs ☠️) and somewhat rare if-then flow logic.
- This tight coupling of the report types makes it really difficult to follow the flow of steps that execute, and it's also just bad software design.
- Further, there are sequence calls set by expression depending on the report type, making it even harder to follow. I'm guessing that this was supposed to be extensible by adding more ReportGen_<type>.seq files, but we're long, long past that being realistic.
- Nomenclature is often inconsistent
- Naming of steps often doesn't match what's really happening.
- Preconditions and post action conditions are very often implicit.
AllOf( This, that, AnyOf( Sometimes the other, or another still), ! The other options that might apply)
&& !( Other implicit conditions because post actions in prior steps precluded execution of this step )
Highly technical details follow:
First off, in the TestReport callback, if report generation is disabled, the message to display is stored in Locals.ReportSection.AsReportSection.Body. Tricky, tricky, you didn't fool me! 😄 Yet, why not use an appropriately named variable?
Also, in the configuration dialog, the report style is referred to as the "Report Format", but the report format in the code is independent of the report style. The nomenclature isn't consistent. Along this theme, in the c code, there is a struct for ReportOptions, but it's not the report options from the TestStand sequence, and it's gradually populated over several consecutive function calls.
Taking ATML, for example, depending on which ATML standard is selected, the execution flow varies substantially, is inconsistent, counterintuitive, and seemingly arbitrary:
ATML 2.02
- In the TestReport callback, if and only if the ATML standard is 2.02, the step "Get Report based on ReportStyle" is called.
- ...but it's only for one style, and not even a style, which would be "xml"
- This Get<report type>Report (GetATMLReport) sequence only exists for ATML (no other formats) and is only called for ATML 2.02, thereby completely removing any justification for a sequence call by expression.
- GetAtmlReport then calls one of two functions in ATMLSupport.dll, depending on whether the caller has initialized a report section
- The report section is always initialized two calls up in "Model Plugin - UUT Done", so there is one case with a DLL call that never executes and is dead code.
- Ultimately, the Get_Atml_ReportSection function is called in ATMLSupport.dll.
- In ATML_Report.c, this calls Get_Atml_Report_Impl, which is where most of the business of appending report strings takes place
ATML 5.00
In the TestReport callback:
- Locals.ReportGenSequenceFileName is determined by looking for the first four characters of FileGlobals.ATML202StandardReportStyle. (i.e. ATML) This is regardless of the ATML version, even though there are respective constants for each ATML version. You might incorrectly assume that it only applies to ATML 2.02
- This time, by precondition we skip execution of "Get Report based on ReportStyle", which is a misnomer and exclusively for ATML 2.02
- Counterintuitively, we also skip "Get Report Body (DLL)", even though we've configured body generation to be via DLL. By precondition, this step is implicitly for any format other than "xml", which I guess means ASCII and HTML? But they're not explicitly named.
- No, we execute "Get Report Body (Sequence)" instead, even thought we're configured to use a DLL. Via various preconditions and post actions, this step is implicitly: Not for ATML 2.02, not for ASCII, and not for HTML. So I guess that means ATML 5.00, ATML 6.01 and XML?
- "Get Report Body (Sequence)" is yet another call by expression to an external reportgen_<format>.seq
- "Get Report Body (Sequence)", when called for these XML based report styles, does not use a sequence. In all of those cases, the AddReportBody sequence it calls calls a DLL and only has half a dozen steps, so it's not a sequence, really.
- Within AddReportBody, there are two possible DLL calls based on whether Parameters.ReportSection is null, but it's never null because the calling sequence always initializes Parameters.ReportSection. (More dead code)
I thought about talking about ATML 6.01, and I could go on, and on... and on. I think you all get the idea.
This is an absolute nightmare to navigate and very messy to customize without an extreme makeover crying edition.
Surely we can do better.
Thank you for reading my missive.
Mr. Jim
)
