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

Add group filter options for /tests #5401

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Add group filter options for /tests #5401

merged 1 commit into from
Dec 22, 2023

Conversation

kraih
Copy link
Member

@kraih kraih commented Dec 19, 2023 •
edited
Loading

Based on #5388, and reusing most of the same logic, to bring job group filtering to /tests from /tests/overview. Similar to the ?match=... option, this does not have a UI element yet and can only be used by manually entering the query parameter. Since the question of how to extend the filter form with text fields is still open, i consider it out of scope for this PR.

Progress: https://progress.opensuse.org/issues/134933

@kraih kraih force-pushed the k/test_group_filters branch 2 times, most recently from b953c0b to 66a6b30 Compare December 19, 2023 14:26
@codecov Codecov
Copy link

codecov bot commented Dec 19, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4cd34a) 98.37% compared to head (69e8e80) 98.37%.
Report is 15 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #5401   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         389      389           
  Lines       37548    37613   +65     
=======================================
+ Hits        36936    37001   +65     
  Misses        612      612

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kraih kraih force-pushed the k/test_group_filters branch from 66a6b30 to b1c6766 Compare December 21, 2023 15:55
@kraih kraih marked this pull request as ready for review December 21, 2023 15:58
kalikiana
kalikiana approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

The naming seems... awkward, but it looks pretty good and I honestly don't have a better suggestion so I guess I'm fine with it

@kraih
Copy link
Member Author

kraih commented Dec 21, 2023

Naming is at least consistent with the other PR.

okurz
okurz requested changes Dec 22, 2023
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Care to add some documentation or help text?

@kraih
Copy link
Member Author

kraih commented Dec 22, 2023

Care to add some documentation or help text?

Where and in what form? I believe the ?match=... and ?groupid=... query parameters are also undocumented for /tests.

@okurz
Copy link
Member

okurz commented Dec 22, 2023

Care to add some documentation or help text?

Where and in what form? I believe the ?match=... and ?groupid=... query parameters are also undocumented for /tests.

That is correct. How about to mention them all as hover text on the "Search:" label like visible on https://openqa.opensuse.org/tests/ ?

@kraih
Copy link
Member Author

kraih commented Dec 22, 2023

Care to add some documentation or help text?

Where and in what form? I believe the ?match=... and ?groupid=... query parameters are also undocumented for /tests.

That is correct. How about to mention them all as hover text on the "Search:" label like visible on https://openqa.opensuse.org/tests/ ?

Sure, but not in this PR. I do not want to touch unrelated features here and create a scope creep situation.

@kraih
Add group filter options for /tests
69e8e80 
Based on #5388, and reusing most of the same logic, to bring job
group filtering to `/tests` from `/tests/overview`. Similar to the
`?match=...` option, this does not have a UI element yet and can
only be used by manually entering the query parameter. Since the
question of how to extend the filter form with text fields is still
open, i consider it out of scope for this PR.

Progress: https://progress.opensuse.org/issues/134933
@kraih kraih force-pushed the k/test_group_filters branch from b1c6766 to 69e8e80 Compare December 22, 2023 10:51
@kraih
Copy link
Member Author

kraih commented Dec 22, 2023

I've expanded the tooltip at the bottom of /tests for group_glob and not_group_glob:
popup

okurz
okurz approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

perfect!

@mergify mergify bot merged commit 857d74f into master Dec 22, 2023
18 checks passed
@mergify mergify bot deleted the k/test_group_filters branch December 22, 2023 11:20
kraih added a commit that referenced this pull request Dec 22, 2023
@kraih
Hanlde no job group filter matches gracefully
fe218a4 
This is a followup for #5388 and #5401. When there were no matches for
job group globs, no query condition would be generated previously. Giving
the false impression that all job groups were matching. So now we just
generate an impossible query that cannot match anything with the group
id `0`.

Progress: https://progress.opensuse.org/issues/134933
@kraih kraih mentioned this pull request Dec 22, 2023
Merged
kraih added a commit that referenced this pull request Dec 22, 2023
@kraih
Handle no job group filter matches gracefully
220ada2 
This is a followup for #5388 and #5401. When there were no matches for
job group globs, no query condition would be generated previously. Giving
the false impression that all job groups were matching. So now we just
generate an impossible query that cannot match anything with the group
id `0`.

Progress: https://progress.opensuse.org/issues/134933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalikiana kalikiana kalikiana approved these changes

@okurz okurz okurz 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.
3 participants
@kraih @okurz @kalikiana