New feature and enhancement: Test titles, descriptions, tags and generics - #68
New feature and enhancement: Test titles, descriptions, tags and generics#68gmartina wants to merge 11 commits into
Conversation
- Introduced SetTestSuiteDescription and SetTestSuiteBrief procedures for setting descriptions and briefs for test suites. - Updated YAML report generation to include brief and description fields. - Enhanced HTML report generation to display brief information for test suites and test cases. - Added utility functions for formatting YAML scalars and escaping HTML.
…s in test case summaries
…ed HTML formatting
|
@gmartina I had a first round of discussions with @JimLewis and we're both very impressed by the size and complexity of the change: modifying VHDL, taking care of TCL code, modifying the YAML, XML and HTML reports, etc. The contribution is very powerful and adds lots of capabilities to OSVVM. Especially as reports are one of the strongest features of OSVVM compared to other frameworks. Your code example uses I have 3 modification requests for the YAML format:
I'll have a look if we can modify the HTML tables with a little bit of embedded JS to support sorting/filtering. |
|
I like what I see. There is alot here. I have family visiting until Monday. I will try to do a bigger review after that. Test tags sound like a good addition. I have been playing with deferred constants as test settings and test tags would give a way to reveal them in the reports. Build Summary report Similar to generics, deferred constants are a good way to control settings in multiple different test cases. As a result, either different test suites need to be used for different settings or test tags would need to In Unit Testing (the low level stuff that is done for the OSVVM library), a test suite is the set of test cases for the package. For core/chip level testing, each testbench has a test harness and many test cases that use that test harness. |
|
@gmartina @Paebbels @Paebbels WRT @Paebbels suggestion for the tags data structure, I agree. I also like: I don't like adding to many levels unless they are doing something useful: |
@gmartina provided testcases for PoC's fifo_cc_got:
As I understood @gmartina, this was one of the driving forces behind the naming and tagging idea, so parameterized tests can be organized in a better way.
The YAML syntax is very clear about that by using the Nested list: key:
- item 1
- item 2Text block with markdown: key: |
- bullet point 1
- bullet point 2Command line arguments on multiple lines: ghdl: >
-a
--std=08
-fexplicitIf it's easier for processing in VHDL (instead of replacing SetTestDescription((
to_line("xnxa asxsanx asknxkanx"),
to_line("# some more text")
));Then no splitting and replacing is required in VHDL, just iterating the array of lines (string pointers) and adding an indentation in front of them.
OK. |
You can choose what to show in the Test Suite report: Same table format when showing Generics and Tags is disabled and there is no Brief:
PoC's fifo_cc_got is a good example for the Generics. You can have different tests with all combinations of Generics. Tags can be used to organize and report complex tests without using generics. It can be added in the Test Controller stimuli process. I find this very useful for very complex designs with a lot of inputs, corner cases, combinations of scenarios... Otherwise, the current solution would be having test file names like: |
Do you mean in the HTML?. The description is shown in the HTML, but in one screenshot, the description section is collapsed. |
Never mind. I was referring to the pull request description :). I started reading/reviewing the actual code and found that not "all" high level additions are mentioned. I'm now step by step into that huge contribution. |
Summary of updated PR with your suggestions
Problem: We need \ for titles (#) Brief: "This is a short summary of TestDescription_Demo1"
Description: |
This demo shows how to document a test case using a *formatted* description.
The goal is to keep it readable in both YAML and HTML.
\## What this test does
- Exercises a simple pass case
- Demonstrates **bold** and *italic* emphasis
- Demonstrates bullet lists
- Demonstrates inline code like `AddTestTag("Name", Value)`
- Demonstrates links like [OSVVM docs](https://osvvm.github.io)
- Demonstrates links like [Other link](/home/user/local/path/example)
Tags:
"Cfg": {Value: "CFG_2", Type: "TAG_STRING", Visibility: {Summary: false}}
"Scenario": {Value: "Maximum Burst", Type: "TAG_STRING", Visibility: {Summary: false}}
"Priority": {Value: "Medium", Type: "TAG_STRING", Visibility: {Summary: false}}
"Condition": {Value: "Back-pressure Active", Type: "TAG_STRING", Visibility: {Summary: false}}
"datawidth": {Value: 8, Type: "TAG_INT", Visibility: {Summary: false}}
"burstlength": {Value: 256, Type: "TAG_INT", Visibility: {Summary: true}}
"syncregs": {Value: true, Type: "TAG_BOOL", Visibility: {Summary: true}}
"bufferenable": {Value: false, Type: "TAG_BOOL", Visibility: {Summary: true}}
"delay_time": {Value: "100 ns", Type: "TAG_TIME", Visibility: {Summary: true}}
"cfg_real": {Value: 3.14, Type: "TAG_REAL", Visibility: {Summary: true}}
Now we don't need LF: ClearTestDescription ;
DescLn("This demo shows how to document a test case using a *formatted* description.") ;
DescLn("The goal is to keep it readable in both YAML and HTML.") ;
DescLn("") ;
DescLn("## What this test does") ;
DescLn("- Exercises a simple pass case") ;Other suggestions:
I tried to implement this but had some type problems. Maybe I revisit this approach in the future. |
What problems did you have? Maybe I can help. type line_vector is array(natural range <>) of line;
type line_vectorP is access line_vector;
procedure SetTestDescription(lines : line_vector);
procedure SetTestDescription(lines : line_vector) is
begin
description = new line_vector'(lines);
end procedure;
function to_line(value : string) return line;
function to_line(value : string) return line is
begin
return new string'(value);
end function;I did not test, just wrote down my idea. |
|
@Paebbels In particular, I do not think the user should have to add extra decoration: Although the to_line would be interesting as overloading. Since this is going to the YAML file, why not use the YAML (or is that tcl) new line character "\n" and just include it in the string handling? I don't see the YAML file as something that humans read. So I am ok with the value being a single string in YAML. If we switched to \n as the new line, then the whole string could go into the YAML unmodified. It is not clear to me that we need to remove accidental characters from the text - is *.md going to throw an error if it sees a CR or HT in the file? |
|
Late yesterday I had the realization that this sort of information is separate from the AlertLogPkg. I think the reason it was put there is because SetTestName is there. SetTestName is in AlertLogPkg because the TestName is the name of the top level AlertLog bin, so it had to be there. I realize that the proposal includes an update to SetTestName with an addition of the optional SetTestTitle. That can be managed a number of ways: @gmartina Do you want to propose a package name? OSVVM uses a Pkg suffix in the name. This would also simplify organizing the unit tests for the package. |
|
@gmartina Is it the printing of the YAML file that had you add this to AlertLogPkg? If the new package is analyzed before AlertLogPkg, AlertLogPkg could call it when it is doing its printing or EndOfTestReports could call the printing from the new package just after calling the printing for the AlertLogPkg. I think I like calling it from EndOfTestReports best - this is in ReportPkg.vhd. |
|
The advantage of decoupling the VHDL API from AlertLogPkg and having EndOfTestReports call the printing for the VHDL API separately is that we can pull the package with little to no risk to AlertLog capability. Likewise when updating the package we do not have to consider any side-effects on AlertLogPkg. Outside of OSVVM internals, when one references the OSVVM library they use the context clause: So to deploy the new package, OSVVM simply includes it in the context. |
|
@JimLewis @gmartina I think a new package sounds reasonable. I also came to the conclusion, that OSVVM has no central "configuration settings" data structure for the top-level name (also used by AlertLog), the title, description, tags, etc. AlertLog is a big piece, but its one island besides coverage, random, scoreboards, etc. So maybe that PR is a good starting point for introducing a new central part of OSVVM holding these information. As Jim mentioned, the SetTestName(..., ....) as an overload from that package sounds very good. The question is, these modifications go beyond a normal contribution via PR. I suggest to merge it into a @gmartina if other contributions are changes come to your mind with respect to this feature set, you could still propose PR from your (new?) branch to |
|
Using the following code is preferred: SetTestDescription(
"This demo shows how to document a test case using a *formatted* description." & LF &
"The goal is to keep it readable in both YAML and HTML." & LF &
LF &
"## What this test does" & LF
);The 2nd proposal was just an idea, to maybe simplify string handling. You cannot emit
A string can be split at While YAML is an intermediate machine-readable format, it's also a human-readable format. For debugging and other investigative processes, we should keep such a file as human-readable as possible. Especially, as it's quite simple to create the required output format. Why should we deliberately create something not human-readable? We could also think about writing a YAML package / protected type, which is handling the file format. |
|
This change would be quite helpful. I am currently working on a testbench where I have to vary multiple generics, such as (Pseudocode) This alone results in 252 possible combinations and a structured overview with the possibility to filter would help a lot. |
| # ClearTestSuiteDescription | ||
| # Clears any previously set suite-level description. | ||
| proc ClearTestSuiteDescription {} { | ||
| set ::osvvm::TestSuiteDescription "" |
There was a problem hiding this comment.
Initialize TestSuiteDescription in OsvvmSettingsRequired.tcl
| # GetTestSuiteDescription | ||
| # Returns the currently configured suite-level description. | ||
| proc GetTestSuiteDescription {} { | ||
| if {[info exists ::osvvm::TestSuiteDescription]} { | ||
| return $::osvvm::TestSuiteDescription | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
If this is initialized in OsvvmSettingsRequired.tcl, then this simplifies to:
| # GetTestSuiteDescription | |
| # Returns the currently configured suite-level description. | |
| proc GetTestSuiteDescription {} { | |
| if {[info exists ::osvvm::TestSuiteDescription]} { | |
| return $::osvvm::TestSuiteDescription | |
| } | |
| return "" | |
| } | |
| # GetTestSuiteDescription | |
| # Returns the currently configured suite-level description. | |
| proc GetTestSuiteDescription {} { | |
| return $::osvvm::TestSuiteDescription | |
| } |
There was a problem hiding this comment.
I am new to these modification things. I note that the above modification requires another action as well.
| if {![info exists ::osvvm::TestSuiteBriefMaxLength]} { | ||
| set ::osvvm::TestSuiteBriefMaxLength 120 | ||
| } | ||
| if {$::osvvm::TestSuiteBriefMaxLength > 0 && [string length $Brief] > $::osvvm::TestSuiteBriefMaxLength} { | ||
| puts "Warning: SetTestSuiteBrief length ([string length $Brief]) exceeds TestSuiteBriefMaxLength ($::osvvm::TestSuiteBriefMaxLength)" | ||
| } |
There was a problem hiding this comment.
Any limits should be applied to the VHDL code so the user gets the information earlier.
| if {![info exists ::osvvm::TestSuiteTitleMaxLength]} { | ||
| set ::osvvm::TestSuiteTitleMaxLength 80 | ||
| } | ||
| if {$::osvvm::TestSuiteTitleMaxLength > 0 && [string length $Title] > $::osvvm::TestSuiteTitleMaxLength} { | ||
| puts "Warning: SetTestSuiteTitle length ([string length $Title]) exceeds TestSuiteTitleMaxLength ($::osvvm::TestSuiteTitleMaxLength)" | ||
| } |
There was a problem hiding this comment.
Any limits should be applied to the VHDL code so the user gets the information earlier.
| if {[info exists ::osvvm::TestSuiteTitle]} { | ||
| return $::osvvm::TestSuiteTitle | ||
| } | ||
| return "" |
There was a problem hiding this comment.
Initialize TestSuiteTitle to "" in OsvvmSettingsRequired.tcl. Then simply return the value. Make sure it is set back to "" when the Build finishes or a new TestSuite is introduced.
There was a problem hiding this comment.
There are other places in OSVVM Tcl scripts that use info exists on variables - Iooking back I think that was a mistake and it should be initialized.
| # ------------------------------------------------- | ||
| # Brief length guard (soft warning) | ||
| # 0 disables warning. | ||
| variable TestSuiteBriefMaxLength 120 | ||
|
|
||
| # ------------------------------------------------- | ||
| # Test Case Summary (HTML) column limits | ||
| # These caps prevent the summary table from becoming too wide when many generics/tags exist. | ||
| # 0 (or negative) disables the cap. | ||
| variable TestCaseSummaryMaxGenericsColumns 12 | ||
| variable TestCaseSummaryMaxTagsColumns 12 |
There was a problem hiding this comment.
Set these in OsvvmSettingsDefaults.tcl. Doing so allows OsvvmSettingsLocal.tcl to override them with user settings.
There was a problem hiding this comment.
OTOH, do we care about checking things like this. Yes it potentially mangles the output. Why do I care - the user looks at it, realizes their mistake and updates their code. A similar issue will occur if they choose to use tag names that are silly long in length.
If we suppress some items, they will wonder why it did not print - and potentially add additional ones to try to understand the issue.
There was a problem hiding this comment.
Honestly, it is not clear to me how the Test Case summary can accommodate any test tags or generic values. The problem is that each test case can have a different number of generics / tags. So the report generator is going to have to read all of the tests in that test suite, calculate the maximum number and whether a test case uses them or not, there will be space in the report for them.
We also have the test brief, which could include this sort of information in the build reports.
| set Escaped [string map [list "\\" "\\\\" "\"" "\\\"" "\n" "\\n" "\r" "\\r" "\t" "\\t"] $Value] | ||
| return "\"$Escaped\"" | ||
| } | ||
|
|
||
|
|
||
| # ------------------------------------------------- | ||
| # FormatYamlDoubleQuotedScalar | ||
| # Always returns a double-quoted YAML string (never native/null). | ||
| proc FormatYamlDoubleQuotedScalar {Value} { | ||
| set Value [SanitizeTextForReport $Value] | ||
| set Escaped [string map [list "\\" "\\\\" "\"" "\\\"" "\n" "\\n" "\r" "\\r" "\t" "\\t"] $Value] |
There was a problem hiding this comment.
Is this there so that the embedded markdown text preserves the control characters?
| puts $YamlFile "${Prefix}${DictName}:" | ||
| foreach {Name Value} $DictValues { | ||
| puts $YamlFile "${Prefix} ${Name}: \"$Value\"" | ||
| foreach Name [lsort -dictionary [dict keys $DictValues]] { |
There was a problem hiding this comment.
Do we want to sort the dictionary or do we want to present parameters in the order they were entered?
This will change backward compatibility with OSVVM's current reports which present parameters in the order they were presented. Probably not a problem since currently OSVVM does not self-check this sort of information. I don't know if anyone else cares or not.
| regsub -all {[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]} $Text { } Clean | ||
| return $Clean | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are we worried about these? The text we have comes from VHDL and in VHDL it is hard to get non-printing characters - it has to be deliberate.
| set CandidateTitle [string trim [dict get $Alert2HtmlDict Title]] | ||
| if {$CandidateTitle ne ""} { | ||
| set DisplayName $CandidateTitle | ||
| } | ||
| } |
There was a problem hiding this comment.
I see you are using "string trim" with the title. Are we expecting leading or training white space that needs to be removed?
SetTestTitle(" ABC ");
|
The comments that were just posted, I had started quite some time ago. |
4 participants
Summary
This PR enhances OSVVM report metadata and presentation across YAML, HTML, and JUnit XML outputs.
New metadata features
Report generation changes
Title,Brief,Descriptionwhen set.Title,Brief,Description,Tags, plus tag visibility metadata.burstlength: 256), so report generators can preserve type information.title/briefproperties when present.Configuration & formatting utilities
VHDL API additions (OSVVM/AlertLogPkg.vhd)
SetTestTitle,GetTestTitle,ClearTestTitle.SetTestName:SetTestName(Name, Title => "...")to set a stable identifier plus an optional display title.SetTestBrief,GetTestBrief,ClearTestBrief.SetTestTag(TagName, TagValue, ShowInSummary => ...)overloads for common types (string/boolean/integer/time/real/std_logic), including control over whether each tag appears in summary tables.Usage
Screenshots
Test Suite Summary table:
Added test case summary table:
Markdown subset example:
Added Generics table:
Added Tags table
Test case report
YAML Output:
Related PR
OSVVM/OSVVM#105
Related Issues
#67