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

test(internal): improve test coverage #4779

Merged
merged 8 commits into from
May 21, 2024
Merged

Conversation

mbhrznr
Copy link
Contributor

@mbhrznr mbhrznr commented May 20, 2024

working towards #3713.
pushes code coverage of the internal sub-module closer to 100%.

the only file missing is internal/styles.ts, which is vendored from fmt/colors.ts.
added a test case for noColor, yet that doesn't seem to be sufficient to appease codecov.

@mbhrznr mbhrznr requested a review from kt3k as a code owner May 20, 2024 16:02
Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (16162eb) to head (2497e99).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4779      +/-   ##
==========================================
- Coverage   91.88%   91.72%   -0.17%     
==========================================
  Files         486      486              
  Lines       41337    41325      -12     
  Branches     5325     5294      -31     
==========================================
- Hits        37984    37905      -79     
- Misses       3296     3357      +61     
- Partials       57       63       +6     

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

Copy link
Contributor Author

@mbhrznr mbhrznr left a comment

Choose a reason for hiding this comment

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

@iuioiua i would be really keen to get your feedback on this one.

internal/diff.ts Show resolved Hide resolved
internal/diff.ts Show resolved Hide resolved
internal/diff_str.ts Show resolved Hide resolved
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Excellent work so far. This is great.

internal/diff_str.ts Outdated Show resolved Hide resolved
@@ -112,3 +111,9 @@ export function buildMessage(
messages.push(...(stringDiff ? [diffMessages.join("")] : diffMessages), "");
return messages;
}

/** Used internally for testing. */
export const _internals = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's export these functions instead of putting them in these _internals objects. It'll be much cleaner. Ditto for the other files in this package. This is fine to do here as this is purely an internal package. You'll have to add @example tags to the newly exported symbols to make the doc checker (deno task lint:docs) too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha!
for some reason i was unsure if we would ever want to export the other functions in this package.
implemented as suggested.
love the way how the doc checker works bth!

Copy link
Contributor

@iuioiua iuioiua May 21, 2024

Choose a reason for hiding this comment

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

Are you able to fix the functions in this file too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh... i'm so sorry.
cleanup for build_message is done as well.

@mbhrznr mbhrznr requested a review from iuioiua May 21, 2024 06:48
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Looking better.

internal/diff.ts Outdated Show resolved Hide resolved
@mbhrznr mbhrznr requested a review from iuioiua May 21, 2024 08:16
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Thank you very much for this. We should discuss and explore options for hitting that colors code.

@iuioiua iuioiua merged commit a5bc643 into denoland:main May 21, 2024
12 checks passed
@mbhrznr mbhrznr deleted the test/internal branch May 21, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants