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] properly determine length of rootValids for engine in isSame comparison #23

Merged
merged 1 commit into from
Oct 17, 2021

Conversation

travi
Copy link
Contributor

@travi travi commented Sep 18, 2021

in attempting to understand why our newly added ls-engines checks were failing for semantic-release/npm#402 and semantic-release/github#412, i tracked down that the graphValids and rootValids were the same lists, but not being reported as such.

i havent tracked down how to cover this gap in the tests yet, but wanted to get this much out here before calling it a (late) night. i'm happy to help get a test in place for this with a little guidance.

Co-authored-by: Matt Travi <programmer@travi.org>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb
Copy link
Owner

ljharb commented Sep 18, 2021

woof, that’s an obvious bug. I’ll take a look this weekend and see if i can come up with tests.

@codecov
Copy link

codecov bot commented Sep 18, 2021

Codecov Report

Merging #23 (c0d5356) into main (b91f667) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #23   +/-   ##
=======================================
  Coverage   82.35%   82.35%           
=======================================
  Files           5        5           
  Lines          85       85           
  Branches       26       26           
=======================================
  Hits           70       70           
  Misses         15       15           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b91f667...c0d5356. Read the comment docs.

@travi
Copy link
Contributor Author

travi commented Sep 20, 2021

I’ll take a look this weekend and see if i can come up with tests.

thanks for taking a look. having looked through the test suite, it does seem like you would be better suited to address the tests on this one, but let me know if it would be worth me digging deeper.

@ljharb
Copy link
Owner

ljharb commented Sep 20, 2021

I'll definitely take a crack at it before asking you to do more work :-) thanks!

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks! I added tests, and will land and release this shortly.

@ljharb ljharb marked this pull request as ready for review October 17, 2021 05:15
@ljharb ljharb merged commit c0d5356 into ljharb:main Oct 17, 2021
@travi travi deleted the is-same branch October 30, 2021 05:26
@travi
Copy link
Contributor Author

travi commented Oct 30, 2021

just getting back from vacation, so this was a great thing to come back to. thanks so much for taking care of this!

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