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: handle edge case when formatting reporter titles #712

Merged
merged 2 commits into from
May 11, 2023

Conversation

clample
Copy link
Contributor

@clample clample commented May 10, 2023

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Resolves after release #697

The linked issue reports checkly trigger causing an out of bounds error. This appears to happen in a CI environment:

RangeError: Invalid count value: -4
    at String.repeat (<anonymous>)
    at formatSectionTitle (/home/circleci/.npm/_npx/a66ee913fe899ce0/node_modules/checkly/src/reporters/util.ts:234:39)
...

This is caused by the terminal width being 0 in some CI environments (aws/aws-cdk#2253).

When we format the section title, we then get a negative number and cause this out of bounds error:

return `──${chalk.bold(title)}${'─'.repeat(width - title.length)}`

To fix the issue, this PR uses Math.max(..., 0) to ensure that the padding length is always non-negative. When working on the PR, I also noticed that the formula doesn't take the indentation and extra left-side -- into account when calculating the right padding. This causes always 6 characters of wrap-around in the title (see first screenshot below). This PR also updates the calculation for the right padding to take this into account and avoid the wraparound (see second screenshot below).

Before PR:
Screenshot 2023-05-10 at 13 23 08

After PR:
Screenshot 2023-05-10 at 13 15 26

@clample clample force-pushed the chrislample/add-edge-case-handling-to-title-format branch from ea144bc to a86c3a9 Compare May 10, 2023 11:43
@clample clample force-pushed the chrislample/add-edge-case-handling-to-title-format branch from a86c3a9 to d10ce66 Compare May 10, 2023 11:45
@clample clample requested a review from nahuelon May 10, 2023 11:48
Copy link
Contributor

@nahuelon nahuelon left a comment

Choose a reason for hiding this comment

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

LGTM

@clample clample merged commit 45e3cfa into main May 11, 2023
@clample clample deleted the chrislample/add-edge-case-handling-to-title-format branch May 11, 2023 08:59
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