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.

Sign up for GitHub

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

Jump to bottom

feat(test): Betelgeuse Docstrings Validation workflow #280

Merged
merged 1 commit into from
Sep 2, 2024
Merged

feat(test): Betelgeuse Docstrings Validation workflow #280

merged 1 commit into from
Sep 2, 2024

Conversation

zpetrace
Copy link
Contributor

No description provided.

@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from f93da93 to 9a14dcc Compare August 28, 2024 13:46
ptoscano
ptoscano reviewed Aug 28, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

  • integration-tests/.github/workflows/polarion.yml -- this won't make the new workflow used, as it must be in the .github/workflows/ directory
  • please name the file something more explicit than "polarion"
  • since betelgeuse and testimony do different jobs, I'd create two different jobs in this new workflow (one for each)
  • the actual config files are missing

@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch 3 times, most recently from da9460a to d2d4d4a Compare August 28, 2024 14:39
@zpetrace zpetrace marked this pull request as draft August 28, 2024 14:43
@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch 4 times, most recently from eb76be9 to 54cbaa1 Compare August 28, 2024 16:04
@zpetrace zpetrace requested a review from ptoscano August 28, 2024 16:06
ptoscano
ptoscano reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

also make sure to

  • not have trailing whitespaces in lines in the newly added files
  • end the newly added files with an empty new line

.github/workflows/docstring_validation.yml Outdated Show resolved Hide resolved
.github/workflows/docstring_validation.yml Outdated Show resolved Hide resolved
.github/workflows/docstring_validation.yml Outdated Show resolved Hide resolved
@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from 54cbaa1 to ae7ffc9 Compare August 29, 2024 10:38
@zpetrace zpetrace requested a review from ptoscano August 29, 2024 10:39
@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from ae7ffc9 to 4801534 Compare August 29, 2024 10:47
@zpetrace zpetrace marked this pull request as ready for review August 29, 2024 12:31
@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from 4801534 to 195018d Compare August 29, 2024 13:12
@ptoscano
Copy link
Contributor

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

More notes from my side:

  • the testimony validate job right now fails because no test has the testimony docstring comments; sadly github does not really have a way to mark a job as "expected to fail", so we need to think about what to do in the meanwhile:
    1. let the job fail; I personally find this ugly, because it will be a "failing" job in the pipeline, and thus adding noise in PRs
    2. run testimony ignoring its return status (something like foo || true); ugly as well, and will need manual checking to see whether there were new problems or the tests were fixed
    3. 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
    4. something else I haven't thought about
  • in the configuration of testimony, IMHO some of the fields would be better as choice type, setting the valid values:
    • SubSystemTeam would have only sst_csi_client_tools as value for now, and more team names would be added as needed
    • Tier seems like it would accept Tier 0, Tier 1, and so on
  • what is the Upstream field for? in the currently open PRs, it seems always set to Yes
  • what is the CaseAutomation field for? in the currently open PRs, it seems always set to Automated
  • what is the CaseComponent field for? in the currently open PRs, it seems always set to insights-client
  • AFAIWT (As Far As I Was Told) tests ought to have an UUID, and thus a id field?
  • what about adding a References (or whichever name is better) to list any references for a test? for example bug tracker IDs (e.g. bugzilla, Jira, etc), generic URL to documentation, and so on; this way, it would be easy to "link" resources to tests
    • I see that existing URLs are removed in the currently open PRs, and I would rather have them

@zpetrace
Copy link
Contributor Author

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.

@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch 3 times, most recently from 48d3795 to 9663d06 Compare August 29, 2024 14:37
@ptoscano
Copy link
Contributor

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
Copy link
Contributor Author

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.

@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from 9663d06 to 75ce928 Compare August 30, 2024 10:18
@ptoscano
Copy link
Contributor

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 zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from 75ce928 to 804ad1b Compare August 30, 2024 12:28
@zpetrace
feat(workflow): Adding a GitHub workflow for test docstrings validation
23949fb 
- Introduced a new GitHub Actions workflow (`docstring_validation.yml`) to validate docstrings using Betelgeuse. This workflow triggers on pull requests affecting the `integration-tests/` directory and performs a dry run with Betelgeuse.
- Added a `README.md` to the `integration-tests/` directory, documenting how to run Betelgeuse for generating and importing test-case and test-run XML files.
- Created a custom Betelgeuse configuration (`custom_betelgeuse_config.py`) to define additional fields for test cases, ensuring proper parsing of docstrings in the `integration-tests/` directory.
@zpetrace zpetrace force-pushed the zpetrace/betelgeuse_workflow branch from 804ad1b to 23949fb Compare August 30, 2024 12:32
@zpetrace
Copy link
Contributor Author

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

@ptoscano
Copy link
Contributor

/packit retest-failed

1 similar comment
@ptoscano
Copy link
Contributor

ptoscano commented Sep 2, 2024

/packit retest-failed

@ptoscano ptoscano merged commit 8947089 into master Sep 2, 2024
19 of 20 checks passed
@ptoscano ptoscano deleted the zpetrace/betelgeuse_workflow branch September 2, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptoscano ptoscano ptoscano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zpetrace @ptoscano