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

Flatten testsets during report generation #104

Merged
merged 6 commits into from
May 1, 2024
Merged

Conversation

omus
Copy link
Member

@omus omus commented Apr 29, 2024

Previously we would perform the testset flattening once we finished the top-level ReportingTestSet. This PR changes the flattening to occur when report is called which is where it is necessary for JUnit XML report generation. Doing this allows introspecting of the nested ReportingTestSet structure and opens up the possibility of using this testset in other report generation which does support nesting.

Additionally, the code changes here eliminates the need for handle_top_level_results!.

@omus omus requested a review from oxinabox April 29, 2024 16:23
Comment on lines +244 to +246
if depth > 0 || has_description(ts)
childts.description = ts.description * "/" * childts.description
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we only skip appending unnamed top-level test sets from the nested testset description. I think it would be reasonable to skip any unnamed testset at any level but didn't want to work through all of the repercussions of such a change at this time.

Copy link
Member

Choose a reason for hiding this comment

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

I agree i would want to think about that a little more

@omus
Copy link
Member Author

omus commented Apr 29, 2024

CI isn't running as this doesn't use master as the base branch currently. Will mark this as draft for now.

Update: Workflow has been updated to allow for PRs not based on master to run

@omus omus marked this pull request as draft April 29, 2024 16:26
@omus omus marked this pull request as ready for review April 29, 2024 20:40
@omus omus mentioned this pull request Apr 29, 2024
Base automatically changed from cv/flattened-testset-order to master April 30, 2024 15:52
Refactored the code to no longer flatten at the top-level when finishing
the top-level `ReportingTestSet` and instead perform the flattening when
we generate the JUnit XML report. Doing this allows introspecting of the
nested `ReportingTestSet` structure and opens up the possibility of
using this testset in other report generation which does support
nesting.
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -4 to -5
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

for my education can you explain this chagne?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was changed to address this: #104 (comment)

The branches filter for the pull_request event restricts this workflow such that it only runs when a PR targets master (in this case) as the base branch. This PR originally didn't target master but another PR so the CI wouldn't run automatically. I don't see a good reason to restrict the CI workflow in these cases so I removed this.

Comment on lines +244 to +246
if depth > 0 || has_description(ts)
childts.description = ts.description * "/" * childts.description
end
Copy link
Member

Choose a reason for hiding this comment

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

I agree i would want to think about that a little more

@omus omus merged commit 4111d2c into master May 1, 2024
10 checks passed
@omus omus deleted the cv/flatten-refactor branch May 1, 2024 14:16
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.

2 participants