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

feat: changes to success criteria checks display #1943

Open
wants to merge 2 commits into
base: feat/introduce-respect
Choose a base branch
from

Conversation

DmitryAnansky
Copy link
Contributor

What/Why/How?

Add condition information to each SUCCESS_CRITERIA_CHECK display output.

Reference

Testing

Screenshots (optional)

Screenshot 2025-02-21 at 18 59 27

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Copy link

changeset-bot bot commented Feb 21, 2025

⚠️ No Changeset found

Latest commit: 4a00fd9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@DmitryAnansky DmitryAnansky marked this pull request as ready for review February 21, 2025 17:03
@DmitryAnansky DmitryAnansky requested a review from a team as a code owner February 21, 2025 17:03
Copy link
Contributor

github-actions bot commented Feb 21, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 79.16% 6916/8737
🟡 Branches 68.06% 2862/4205
🟡 Functions 74.93% 1139/1520
🟡 Lines 79.52% 6591/8289
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / display-checks.ts
89.58%
48.15% (-1.85% 🔻)
62.5% 89.36%

Test suite run success

1323 tests passing in 189 suites.

Report generated by 🧪jest coverage report action from 4a00fd9

@@ -271,6 +271,7 @@ export type Check = {
name: string;
message?: string;
additionalMessage?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might also reuse additionalMessage, but imho having criteriaCondition is better describes the concept and will be more useful in json logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it looks like additionalMessage is a better option since it serves the same exact purpose for checks derived from OpenAPI description in checkStatusCodeFromDescription.

Comment on lines +41 to +54
check.criteriaCondition.length > MAX_CRITERIA_CONDITION_DISPLAY_LENGTH
? check.criteriaCondition.slice(0, MAX_CRITERIA_CONDITION_DISPLAY_LENGTH) + '...'
: check.criteriaCondition
)}`
: ''
}${check?.additionalMessage ? ` (${check.additionalMessage})` : ''}`;

const failTestMessage = (checkName: string, severity?: RuleSeverity) =>
`${severity === 'warn' ? yellow('⚠') : red('✗')} ${gray(checkName.toLowerCase())}${
check?.additionalMessage ? ' (' + check.additionalMessage + ')' : ''
}`;
check?.criteriaCondition
? ` - ${red(
check.criteriaCondition.length > MAX_CRITERIA_CONDITION_DISPLAY_LENGTH
? check.criteriaCondition.slice(0, MAX_CRITERIA_CONDITION_DISPLAY_LENGTH) + '...'
: check.criteriaCondition
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the common logic to a utility function?

@@ -271,6 +271,7 @@ export type Check = {
name: string;
message?: string;
additionalMessage?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

For me it looks like additionalMessage is a better option since it serves the same exact purpose for checks derived from OpenAPI description in checkStatusCodeFromDescription.

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.

2 participants