Skip to content
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

Initial set of report generator update #105

Merged
merged 16 commits into from
May 21, 2021

Conversation

shigeya
Copy link
Contributor

@shigeya shigeya commented May 16, 2021

Initial work for report generation. Related to Issue #78.

I'm sending this PR to provide the latest status of this work and wish to have feedbacks on the report structure since I'm changing that a lot.

  • Suites configuration is now in suites/suite-config.json. Currently, only provides the list of test suites. The configuration parameters in this file override configurations in each of the suites. Currently, each of the default.js files contains defaults for each of the suites. I think these should be moved to suite-config.json
  • All of the tests tweaked to have the same root ancestors suite-name and implementation name, to make report matrix creation easier. These were included in part of the describe statement, but it's not easily machine-parsable and some of them are inconsistent.
  • All of the long raw outputs are moved to Appendix.
  • Added/updated Terminology section.
  • Replaced with new "Conformance Report" section with three subsections: Matrix, by Test Suites, by Methods.

Preview | Diff

@shigeya shigeya changed the title DRAFT: Initial set of report generation DRAFT: Initial set of report generator update May 16, 2021
@shigeya
Copy link
Contributor Author

shigeya commented May 16, 2021

@OR13 @msporny Please provide feedback. I'm now working on conformance section generation.

@shigeya
Copy link
Contributor Author

shigeya commented May 16, 2021

Remaining tasks:

  • Conformance Report Section
    • Matrix
    • by Suites

Also,

  • Decides to keep On-demand section or not

@shigeya
Copy link
Contributor Author

shigeya commented May 16, 2021

First reworked version for "Conformance by Test Suites" section.

I lack of CSS kung-fu. Maybe, we need adjustments on fonts to be used in the statements and tables.

I'm start feeling there is a small value for the "Conformance by method" section, so commented out for now.

@msporny
Copy link
Member

msporny commented May 16, 2021

@shigeya, this is wonderful, thank you!

A couple of thoughts:

  • The initial report looks great, it's formatted so that it's easy to understand the details. It was what I was hoping someone would produce, so this looks wonderful!
  • It is currently difficult to see which features don't have enough support. For example, the JSON null test only has one implementation that has implemented it. It would be good to see these in a section that lists the "Features Lacking at least Two Implementations".
  • I expect we shouldn't count did:example in the list of implementations? However, if we don't do this, there are a number of JSON-only features that would be dropped.
  • It's difficult to understand why alsoKnownAs has so many implementations supporting it... specifically, Digital Bazaar's implementations don't currently support the feature but it still shows our implementations as supporting alsoKnownAs. That leads me to believe that their might be a bug in the tallying code.
  • We shouldn't check index.html or did-spec.latest.json into the github repo -- we shouldn't check in auto-generated files, instead we should invest in running the test suite on a regular basis and publishing to a website, like we did for the vaccination test suite: https://github.com/digitalbazaar/vaccination-certificate-test-suite/runs/2429248801?check_suite_focus=true... this file is generated as a part of the build process: https://w3id.org/vaccination/interop-reports

@shigeya
Copy link
Contributor Author

shigeya commented May 16, 2021

@shigeya, this is wonderful, thank you!

thanks.

  • The initial report looks great, it's formatted so that it's easy to understand the details. It was what I was hoping someone would produce, so this looks wonderful!

Good to hear that.

  • It is currently difficult to see which features don't have enough support. For example, the JSON null test only has one implementation that has implemented it. It would be good to see these in a section that lists the "Features Lacking at least Two Implementations".

Looking through the test report again, it seems there are some cases not handled correctly. Will check them.

  • I expect we shouldn't count did:example in the list of implementations? However, if we don't do this, there are a number of JSON-only features that would be dropped.

We need to remove examples from the report.
Maybe, either keep them as part of tests (then skip from the output) or keep them as an example configured to skip tests, etc.

  • It's difficult to understand why alsoKnownAs has so many implementations supporting it... specifically, Digital Bazaar's implementations don't currently support the feature but it still shows our implementations as supporting alsoKnownAs. That leads me to believe that their might be a bug in the tallying code.

Will check the code.

I agree. Actually, that is my preference. Since I'm relying on Orie's code I kept the flow of the report generation. The current report page contains too much automation in the report. Since the number of tests entry is increasing, the report is getting larger and slower, that's what we don't want to see.

If it's okay, I'll move most of the code from the index.html into the report generator. Also, remove the report JSON in the report, and maybe removing the On Deman Test section.

@shigeya
Copy link
Contributor Author

shigeya commented May 18, 2021

Consolidated parameters into a single column, also showing parameters that show how each of the tests is different.
Looks like did-resolution and did-url-dereferencing sections are still strange, all of the other tables look good.

@msporny

It's difficult to understand why alsoKnownAs has so many implementations supporting it... specifically, Digital Bazaar's implementations don't currently support the feature but it still shows our implementations as supporting alsoKnownAs. That leads me to believe that their might be a bug in the tallying code.

it was not tallying problem but the test code on 5.1.3.
The test code block it succeeds even there is no alsoKownAs property.
Please refer to the commit 4cf9626

But now, the empty table causes the table for alsoKnownAs test to disapper. Need to find a way to fix this.

@shigeya shigeya force-pushed the shigeya-report-generator-work-1 branch from 4cf9626 to ecd3077 Compare May 18, 2021 00:17
@shigeya
Copy link
Contributor Author

shigeya commented May 18, 2021

I will work on moving most of the report generation code in index.html into the report generator code, then good to merge. We can improve later, including the matrices. I don't want to away from up-to-date main branch. I don't have time for this in the next 24hrs, so it will be ready after Tuesday's call.

I leave resolution/derefencing as is for now.

@shigeya shigeya force-pushed the shigeya-report-generator-work-1 branch from ecd3077 to 0bb5f9b Compare May 18, 2021 11:52
@shigeya
Copy link
Contributor Author

shigeya commented May 18, 2021

I cherry-picked PR #110 and PR #111. Will remove them when this PR exits DRAFT.

@shigeya shigeya force-pushed the shigeya-report-generator-work-1 branch from 0bb5f9b to a530b57 Compare May 18, 2021 12:48
@shigeya
Copy link
Contributor Author

shigeya commented May 18, 2021

I leave resolution/dereferencing as is for now.

Fixed.

I'll move the code in HTML to the report generator script next.

@OR13
Copy link
Contributor

OR13 commented May 18, 2021

this looks awesome! great work on this.

@shigeya shigeya force-pushed the shigeya-report-generator-work-1 branch from a530b57 to 8d68435 Compare May 18, 2021 13:57
@shigeya shigeya force-pushed the shigeya-report-generator-work-1 branch from 8d68435 to 3e557c4 Compare May 18, 2021 15:12
@shigeya
Copy link
Contributor Author

shigeya commented May 18, 2021

It was easy. Now it is generating a report by generate-latest-respec-data.js.

Besides the matrix generation, it's done. please review and merge.

For the testing purpose, need PR #110 to test to pass ... to generate a report.

@shigeya shigeya marked this pull request as ready for review May 18, 2021 15:16
@shigeya shigeya requested review from msporny and OR13 as code owners May 18, 2021 15:16
@shigeya shigeya changed the title DRAFT: Initial set of report generator update Initial set of report generator update May 18, 2021
@@ -8,7 +8,7 @@ module.exports = async (suiteName, config) => {
// ci: true,
// silent: true,
roots: [path.resolve(__dirname, `../suites/${suiteName}`)],
globals: JSON.stringify({ suiteConfig: config }),
globals: JSON.stringify({ systemSuiteConfig: config }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for folks wondering, this is the magic of our jest tests... we are injecting a global json object here... which is the same object that an http client can submit to an http web server to generate the report json.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. It was a shock to understand that globals is a jest's option. The name globals is obvious from the feature, but the name is strange for an optional parameter.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, it looks excellent.

@OR13
Copy link
Contributor

OR13 commented May 21, 2021

This PR has been open for 6 days and has multiple approvals, merging.

@OR13 OR13 merged commit 8a2fa15 into w3c:main May 21, 2021
@shigeya shigeya deleted the shigeya-report-generator-work-1 branch May 21, 2021 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants