RedHatInsights / insights-client

insights-client
Other
27 stars 49 forks source link

feat(test): Betelgeuse Docstrings Validation workflow #280

Closed zpetrace closed 3 months ago

ptoscano commented 3 months ago

Thanks for the changes so far, it runs the jobs properly now :)

More notes from my side:

zpetrace commented 3 months ago

Thanks for the notes @ptoscano!

For the configuration of testimony - I can definitely make some of the field choices, that's not a problem (and it would actually be better) and I can definitely add a reference field with string type (as that will be changing from test to test). For the other fields that will always be the same - yes, they will. I made them global for easier readability of the code (so they don't repeat in every test case) but (I think from what I understood) we need those fields for traceability for Polarion. When the test cases will be imported into Polarion they need to have those fields (we know it is automated and it's upstream but we need to see that in Polarion as well as not all test cases added will be from this repo so we need which ones are/aren't automated there). Maybe @Lorquas will have some additional answer to that.

For the testimony validate - I personally would stick to the 3rd option - do not merge the job until all the tests have valid docstring comments; this means running testimony manually in PRs with changes related to this to check the results but that could also take a month so we should consider if we want to have a PR open for that long but IMHO it seems like the best option so far.

ptoscano commented 3 months ago

I can definitely make some of the field choices, that's not a problem (and it would actually be better) [...] For the other fields that will always be the same - yes, they will.

Thanks!

I can definitely add a reference field with string type (as that will be changing from test to test).

Thanks! I'd not make it required thought, as there may not be references for a test, and that's OK (not ideal, still OK).

For the testimony validate - I personally would stick to the 3rd option - do not merge the job until all the tests have valid docstring comments; this means running testimony manually in PRs with changes related to this to check the results but that could also take a month so we should consider if we want to have a PR open for that long but IMHO it seems like the best option so far.

OK, makes sense. In this case, what do you think about splitting the betelgeuse job (and its config) in its own PR? That one seems to work fine already, and we can run it in new PRs to validate the result/output.

zpetrace commented 3 months ago

I'd not make it required thought, as there may not be references for a test, and that's OK (not ideal, still OK).

Yeah sure:)

OK, makes sense. In this case, what do you think about splitting the betelgeuse job (and its config) in its own PR? That one seems to work fine already, and we can run it in new PRs to validate the result/output.

Yeah, that makes sense, I will leave this PR to betelgeuse only then and I will create a separate PR for testimony that I will leave as draft for now.

ptoscano commented 3 months ago

Sounds good.

One thing I'd add here is the README.md as currently added in #270, as IMHO it fits this as more general "Betelgeuse enablement". Don't forget to update it according to the changes that were done here to run betelgeuse properly.

Also, please explain a bit more the changes as commit message, so the content of the commit in this PR is a bit less cryptic, including what it is for.

Lastly: please rebase this branch on top of master, while you are pushing new changes.

Thanks!

zpetrace commented 3 months ago

Rebased, commit message added and README.md added also :)

ptoscano commented 3 months ago

/packit retest-failed

ptoscano commented 3 months ago

/packit retest-failed