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

Different order of parameters in HTML Report #190

Closed
t-huyeng opened this issue Dec 4, 2022 · 19 comments
Closed

Different order of parameters in HTML Report #190

t-huyeng opened this issue Dec 4, 2022 · 19 comments

Comments

@t-huyeng
Copy link
Contributor

t-huyeng commented Dec 4, 2022

First of all great project!

I use vacuum to generate html reports for all the openapis we have created within the bunddev project,
For this I created a Go-script which runs every night and creates a PR if changes of the HTML-reports exist. I already removed the line where the generated time has been set with a small hack. But it looks like the order of parameters is different on some runs.

See this PR (which was created through the github action):
t-huyeng/check-bundesAPI-repos#8

It shows the changes of the html files: as far as I see it the changes are because of a changed order of parameters or other information.

Can you confirm that the order is not always the same? Could you thing about implementing something which keeps the order the same for every run?

Thanks

@daveshanley
Copy link
Owner

daveshanley commented Dec 5, 2022

Hi, Thanks very much for your kind feedback.

Before fixing ordering, I wanted to understand why it's important to you. The HTML report's purpose is to render the results in a valuable, quick, and easy way to view.

However, it sounds like you're performing some diff checks on the files to see what's changed. Because there shouldn't be any impact on the report if things are re-ordered, it will mess things up if you're trying to use the report to determine if something has changed.

If you're trying to determine what has changed between two specs - there is a much more powerful way to do so (rather than using vacuum).

libopenapi has a feature called what changed, and it will provide you with a rich API to determine all the changes found between two specs. You can see an example here: https://github.com/pb33f/libopenapi#discover-what-changed

A brand new tool (prototype only for right now) turns this library into a complete application (like vacuum). It allows changes to be seen across the entire history of a specification (if it's stored in Git). You can see that here: https://github.com/pb33f/openapi-changes (warning, prototype only for now).


Of course, that all being said, if you're not looking to determine what has changed, then I need to know a little more about the ordering requirements for the report.

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 5, 2022

Thanks for the quick reply.

Well I think it is little bit of both.
I will try to explain the use-case a little bit more:
At BundDEV we have more than 40 OpenAPIs which describe various public Open Data Endpoints in Germany. What I would like to achieve is a place for all the OpenAPI reports. For this I created the repo which creates every night via a GitHub Action (which triggers a Go script) vacuum html reports and saves them in a folder (vacuum-reports). After this has been done for all the OpenAPIs another script will create a PR if changes are there. This is helpful if for example a new API description has been added to the project or if somebody updated the OpenAPI (e.g. fixed some Warnings). So yes I kind of want to track the changes or lets say I want to know if something of the html report has changes. I do not really need to know what kind of changes there are.

Maybe this quick drawing helps:
image

  • other checks are just our project specific test (URL, descriptions of github repo etc.)
  • removing the generated div (html line 1457) is just a hack so to prevent more changes of html files

The Readme as well as all the vacuum html reports are hosted via Github-Pages for easy viewing. See Readme-Webpage or example for one report here.

Does this make more sense?

tl;dr: For me changes of the OpenAPI are import if they change the vacuum html report which is hosted via GitHub Pages for al of our OpenAPIs. For this the order of parameter should be consistent (to prevent "fake" changes of the html).

daveshanley added a commit that referenced this issue Dec 5, 2022
An interesting use-case popped up where vacuum is being used as a 'diff check' tool to determine if output has changed. Some of the data was being rendered randomly, due to it not being sorted consistently. This change has been made to ensure that reach run will be identical in order and HTML will always result in identical output.

Also to ensure this is possible (and will validate against a sha256 hash), there is a new `-d` flag available for the `html-report` command. This has been added to the docs at: https://quobix.com/vacuum/commands/html-report/

Signed-off-by: Dave Shanley <dave@quobix.com>
daveshanley added a commit that referenced this issue Dec 5, 2022
An interesting use-case popped up where vacuum is being used as a 'diff check' tool to determine if output has changed. Some of the data was being rendered randomly, due to it not being sorted consistently. This change has been made to ensure that reach run will be identical in order and HTML will always result in identical output.

Also to ensure this is possible (and will validate against a sha256 hash), there is a new `-d` flag available for the `html-report` command. This has been added to the docs at: https://quobix.com/vacuum/commands/html-report/

Signed-off-by: Dave Shanley <dave@quobix.com>
@daveshanley
Copy link
Owner

A very interesting use case!

From version v0.0.41 the html-report should now produce identical output for each run, everything ordered consistently.

You can also use the new -d flag with html-report to disable the timestamp output also, no need for your hack anymore!

The docs have been updated here: https://quobix.com/vacuum/commands/html-report/

Thank you for your help making vacuum a better tool!

t-huyeng added a commit to t-huyeng/check-bundesAPI-repos that referenced this issue Dec 5, 2022
used flag to disable timestamp,
Thanks to @daveshanley, see daveshanley/vacuum#190
@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 5, 2022

Awesome! Thanks!
I will check this now and report back ASAP.

@daveshanley
Copy link
Owner

Glad to see you're using the API! It's a breaking change there, and I wasn't aware anyone was using it - apologies!

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 5, 2022

The flag to disable the timestamp works fine. 🚀
For later references I am calling the

report :=  html_report.NewHTMLReport(specIndex, specInfo, resultSet, stats, true)

like this now. The last bool true sets disableTimestamp.

It looks like there are still difference for different runtimes (See this PR). I will check locally again and see if this is continuous problem. Again Thanks for this awesome tool!

@daveshanley
Copy link
Owner

daveshanley commented Dec 5, 2022

Hmm, interesting, I was running the stripe API, and it was coming out identical each time. Could you link to a couple of the specs you're finding inconsistent and I can try those on a few different runtimes?

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 5, 2022

Yes:
pegel-online-api

Also

It looks like a few of the difference of the html outputs comes from different order of the category-rule-result :
image

@daveshanley
Copy link
Owner

I think I know what the issue is, I will put a fix together in the next 24 hours or so.

daveshanley added a commit that referenced this issue Dec 6, 2022
Examples being the culprit, in large specs with lots of examples, some ordering was failing because there was not enough detail in the sort (it was only checking a couple of properties). Now sorting checks all the way down the model and applies a hash to paths and messages to ensure consistent ordering.

Testing against stripe, k8s, petstore and user supplied specs that were causing issues.

Signed-off-by: Dave Shanley <dave@quobix.com>
daveshanley added a commit that referenced this issue Dec 6, 2022
Examples being the culprit, in large specs with lots of examples, some ordering was failing because there was not enough detail in the sort (it was only checking a couple of properties). Now sorting checks all the way down the model and applies a hash to paths and messages to ensure consistent ordering.

Testing against stripe, k8s, petstore and user supplied specs that were causing issues.

Signed-off-by: Dave Shanley <dave@quobix.com>
@daveshanley
Copy link
Owner

OK, so you should be good to go. The problem was the sorting algorithm wasn't very robust. I was only sorting by line number.

Please try version v0.0.42 and let me know if the issue has been resolved. I did try a couple of your specs that were previously failing and now create identical output. I've also tried with the specs in model/test_specs and they result in identical output also.

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 6, 2022

Thanks!

Looks like it is working much better now. Sadly I am still getting changes for one API (pegel-online-api). I am not sure what is different with this API.

See this PR

Quick check with the pegel-online openapi and this bash script:

vacuum html-report openapi.yml output0.html -d

# save the time where the run is different in a variable
j=0

i=0
until [ $i -gt 200 ]
do
    echo i: $i
    i=$((i+1))
    echo "Vacuum run $i "
    vacuum html-report openapi.yml output$i.html -d
    DIFF=$(diff output$i.html output0.html)
    if [ "$DIFF" != "" ]
    then
        echo "$i is different from the first run"
        cmp output$i.html output0.html
        # diff output$i.html output0.html > diff$i.txt
        j=$((j+1))
    else
        echo "$i is the same as the first run"
    fi
    # remove the output file
    rm output$i.html

done

echo $j "runs were different as the first run"

Gives the following result:

143 runs were different as the first run

This number various with different runs.

I will look further into the changes

It looks like most changes start in line 1795.

@daveshanley
Copy link
Owner

I do love a game of whack-a-mole! Looks like it's more of the same issue, just further down. I will investigate and have a patch available in the next 24 hours.

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 6, 2022

Awesome, to test it 200 times was just a fun run ;)

@daveshanley
Copy link
Owner

This is a little strange to be honest, I am getting completely different results with that same spec, along with running your bash script. Just to ensure I am not going crazy, I added the spec you mentioned to the sample specs in the repo, and created two new tests, a standard benchmark and a deliberate re-creation of the bash script in code as part of the test suite

Tests don't fail, and the bash-script passes fine for me, and it seems run in the pipeline too without issue.

vacuum-output

I am unable to re-create this problem!

To see the new tests:

https://github.com/daveshanley/vacuum/blob/main/benchmarks/html_report_test.go#L42
https://github.com/daveshanley/vacuum/blob/main/benchmarks/html_report_test.go#L77

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 7, 2022

ok, thanks for testing. I will do more testing and see where my error is.

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 7, 2022

It looks like the API your are testing is not the pegel-online-api specification (it is the openapi from DWD - this API works fine for me as well with the bash script)

The openapi for the pegel-online-api (https://github.com/bundesAPI/pegel-online-api/blob/main/openapi.yaml) starts like this:

openapi: "3.0.0"
info:
  contact:
    url: "https://www.pegelonline.wsv.de/"
    name: "Wasserstraßen- und Schifffahrtsverwaltung des Bundes (WSV)"
  version: "1.0.0"
  x-office: "Wasserstraßen- und Schifffahrtsverwaltung"
  title: "Pegel-Online API"
  description:
    "API für das bundesweite Messstellennetz der Wasserstraßen- und Schifffahrtsverwaltung des Bundes.
....
....

I just created a PR to update the openapi to the correct one see #198.

Sorry for the confusion, to many apis and versions flying around..

@daveshanley
Copy link
Owner

After digging deeper, I've discovered a few more issues that I'll need to fix to make this right.

@daveshanley
Copy link
Owner

daveshanley commented Dec 8, 2022

FYI: #199

There were some lower-level issues I needed to resolve before this could be fixed properly. Can you try v0.0.43?

@t-huyeng
Copy link
Contributor Author

t-huyeng commented Dec 8, 2022

It is working!

Went for a fun run with 1000 tries for the bash script:
image

Also everything working now for the initial problem (https://github.com/t-huyeng/check-bundesAPI-repos/actions/runs/3648578748/jobs/6162163700)

Thanks again for the awesome project 🚀 and quick help ❤️ !

@t-huyeng t-huyeng closed this as completed Dec 8, 2022
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

No branches or pull requests

2 participants