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

Fix Checks results filters #1073

Merged
merged 5 commits into from
Dec 22, 2022
Merged

Fix Checks results filters #1073

merged 5 commits into from
Dec 22, 2022

Conversation

jamie-suse
Copy link
Contributor

@jamie-suse jamie-suse commented Dec 22, 2022

Description

Fixes changes to the new checks results filters from bd2f538.

Fixes #1064

filters

How was this tested?

Added new tests for filtering in ExecutionResults.test.jsx.

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hey @jamie-suse ,
The code looks great! Thank you for adding tests ;)
I would appreciate a gif to see it working

error,
expectations,
failedExpectations,
result: health,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change from health to result?
We use health all around the frontend code.
Is it because the predicate function expects that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's correct, predicate() relies on result being present.

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@jamie-suse jamie-suse merged commit 713fba4 into main Dec 22, 2022
@jamie-suse jamie-suse deleted the filter-fix branch December 22, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants