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

Make it possible to include custom columns from TestActions in case, class and package tables #503

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Conversation

jonaslind
Copy link
Contributor

@jonaslind jonaslind commented Mar 8, 2023

Add optional jelly hooks that allow TestActions to contribute custom columns to tables that list CaseResults, ClassResults and PackageResults.

More details in the corresponding enhancement issue:
Fixes #494

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jonaslind
Copy link
Contributor Author

Desired reviewer: @timja

@timja
Copy link
Member

timja commented Mar 9, 2023

can you check the build is green before requesting my review please

@jonesbusy
Copy link
Contributor

Looks also the master branch is broken : https://ci.jenkins.io/job/Plugins/job/junit-plugin/job/master/321/

@jonaslind
Copy link
Contributor Author

jonaslind commented Mar 9, 2023

@timja @jonesbusy thanks for comments!

Yes, the reason the build fails is RequireUpperBoundDeps, and this issue comes from the master branch. It's not introduced by my pull request. For example, I saw the same failure in @timja 's pull request #501 that was merged yesterday:

https://ci.jenkins.io/job/Plugins/job/junit-plugin/view/change-requests/job/PR-501/1/

It looks like the issue has been introduced by dependabot after c789200. At the same time, it looks like @timja is replacing dependabot with renovate?

I'm happy to try and tackle the RequireUpperBoundDeps failure if no-one gets to it before me. Am I expected to fix that in this pull request, or shall it be done separately in a different pull request?

@timja
Copy link
Member

timja commented Mar 9, 2023

Weird given CI passed on that build.

I've switched it to renovate but they are basically the same tools with a slightly different feature set.

If you're happy to do it then a separate PR would be great. Otherwise I'll get to it when I have a chance

@jonaslind
Copy link
Contributor Author

@timja @jonesbusy I've created pull request #505 to work around the RequireUpperBoundDeps failure.

@jonesbusy
Copy link
Contributor

@jonaslind I'm not maintainer of this plugin, but same things happen to me on the skip-notifications-trait-plugin

https://github.com/jenkinsci/skip-notifications-trait-plugin/pull/62/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R59

@jonaslind
Copy link
Contributor Author

Now with the fix from #505, the build is green.

@jonaslind
Copy link
Contributor Author

@timja , do you think you'll be able to review this pull request?

@timja timja requested a review from a team March 14, 2023 13:45
@timja
Copy link
Member

timja commented Mar 14, 2023

At some point yes but I don't have time right now, I've requested review from all the maintainers though

@timja timja changed the title Make it possible to include custom columns from TestActions in the ClassResult table (#494) Make it possible to include custom columns from TestActions in the ClassResult table Mar 15, 2023
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Seems fine, is there a reason you're just doing this for class results and not other ones?

Comment on lines 36 to 37
* <li>classresult-tableheader.jelly: allows additional table headers to be shown in the test class' table of test methods</li>
* <li>classresult-tablerow.jelly: allows additional table cells to be shown in the test class' table of test methods</li>
Copy link
Member

@timja timja Mar 15, 2023

Choose a reason for hiding this comment

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

I think class's is the correct possessive singular:

Suggested change
* <li>classresult-tableheader.jelly: allows additional table headers to be shown in the test class' table of test methods</li>
* <li>classresult-tablerow.jelly: allows additional table cells to be shown in the test class' table of test methods</li>
* <li>classresult-tableheader.jelly: allows additional table headers to be shown in the test class's table of test methods</li>
* <li>classresult-tablerow.jelly: allows additional table cells to be shown in the test class's table of test methods</li>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the grammar fix, will include in next commit!

@jonaslind
Copy link
Contributor Author

jonaslind commented Mar 16, 2023

@timja Thank you for the review!

I only did this for ClassResult because I had no need for hooks in the other tables, but now that you ask it makes sense to me to do it for a few other tables too:

  • The two tables "All Failed Tests" and "All Tests" in MetaTabulatedResult.
  • The table in History.

That'll make custom columns more generic and consistent across pages. It's more future-proof.

I'll prepare a new commit with jelly hooks in these places too as well as unit tests verifying that it works. Please let me know if you can think of another table where it makes sense to add custom columns.

There are list.jelly files for CaseResult, ClassResult and MetaTabulatedResult. I haven't figured out how to navigate to them and can't find any references to them. All unit tests pass even if I delete these files. What are they used for?

@timja
Copy link
Member

timja commented Mar 16, 2023

I think they are as you click through test rests from the class level to the individual test level.

Its often hard to tell where they are used, I generally put some gibberish in each file and click around till I trace them back

@jonaslind jonaslind changed the title Make it possible to include custom columns from TestActions in the ClassResult table Make it possible to include custom columns from TestActions in case, class and package tables Mar 17, 2023
@jonaslind jonaslind requested a review from timja March 17, 2023 11:43
@jonaslind
Copy link
Contributor Author

@timja, here's a new commit with custom columns added to MetaTabulatedResult. Turns out that History is not a TestObject, so adding custom columns to history pages would involve more refactoring. I'm not sure that is motivated unless there is a clear use case for it. So I've skipped that for now.

Please have a look when you have the time. Thanks!

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

looks good, just one docs change missing

@@ -121,4 +121,8 @@ public TabulatedResult blockToTestResult(@NonNull PipelineBlockWithTests block,
public String getChildTitle() {
return "";
}

public String getChildType() {
Copy link
Member

Choose a reason for hiding this comment

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

please add javadoc

Add optional jelly hooks that allow TestActions to contribute custom
columns to tables that list CaseResults, ClassResults and
PackageResults.
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks!

@timja timja merged commit 1be5936 into jenkinsci:master Mar 17, 2023
@jonaslind
Copy link
Contributor Author

Thank you very much @timja for all your help and input!

@jonaslind jonaslind deleted the custom-columns branch March 17, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to include custom columns from TestActions in the ClassResult table
3 participants