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

Re-enable two pending tests #6194

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Re-enable two pending tests #6194

merged 1 commit into from
Jan 24, 2023

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented Nov 22, 2022

Per the discussion in #3319 (comment),
these only temporarily needed to be marked pending until
#3327 was merged.

From the first thread it looks like the intent was to remove the
pending marker, but that accidentally got overlooked.

So this removes the pending marker, and then fixes the failures:

  1. The first was a straightforward change from method to hash value.
  2. The second wasn't raising the expected error... However, this has
    been true since this code was originally committed
    (once Bundler 2: [Prerelease] Implement v2 native functions for LatestVersionFinder #3327 was merged to make the test work). So I deleted the test
    as it added no value.

@deivid-rodriguez
Copy link
Contributor

@jeffwidman Not sure if this is useful? deivid-rodriguez@ec4f8b6

Apparently I worked on this when first contributing to dependabot-core, but never followed up on it.

@jeffwidman jeffwidman force-pushed the remove-two-pending-markers branch 2 times, most recently from 1e482f2 to bdab181 Compare January 23, 2023 21:21
@jeffwidman jeffwidman marked this pull request as ready for review January 23, 2023 21:23
@jeffwidman jeffwidman requested a review from a team as a code owner January 23, 2023 21:23
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

Approving since CI is passing. Tests lgtm

Per the discussion in #3319 (comment),
these only temporarily needed to be marked `pending` until
#3327 was merged.

From the first thread it looks like the intent was to remove the
`pending` marker, but that accidentally got overlooked.

So this removes the `pending` marker, and then fixes the failures:
1. The first was a straightforward change from method to hash value.
2. The second wasn't raising the expected error... However, this has
   been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250)
   (once #3327 was merged to make the test work). So I deleted the test
   as it added no value.
@jeffwidman jeffwidman merged commit 7b41200 into main Jan 24, 2023
@jeffwidman jeffwidman deleted the remove-two-pending-markers branch January 24, 2023 17:59
alcere pushed a commit that referenced this pull request Feb 20, 2023
Per the discussion in #3319 (comment),
these only temporarily needed to be marked `pending` until
#3327 was merged.

From the first thread it looks like the intent was to remove the
`pending` marker, but that accidentally got overlooked.

So this removes the `pending` marker, and then fixes the failures:
1. The first was a straightforward change from method to hash value.
2. The second wasn't raising the expected error... However, this has
   been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250)
   (once #3327 was merged to make the test work). So I deleted the test
   as it added no value.
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.

3 participants