-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Bundler 2: Add native helpers for LockfileUpdater #3326
Conversation
}] | ||
end | ||
|
||
it "updates the dependency's revision" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? 😅 seems like the test is checking if we keep the same revision as the old lockfile? 🤔
Looks like the requirements are not being updated at all so not clear what's being tested. What happens if we do end up updating the requirement? Do we need to unlock the other gems also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was mostly just adapted from the v1 version, but I think what this is supposed to test is that we keep the same remote after the update, but that it does not have the same revision:
expect(new_remote_line).to eq(original_remote_line)
expect(new_revision_line).to_not eq(original_revision_line)
This seems right to me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes you're right, totally missed the to_not eq
bit! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's chance it to not_to
👍
expect { updater.updated_dependency_files }. | ||
to raise_error(Dependabot::NotImplemented, | ||
"Bundler 2 adapter does not yet implement update_lockfile") | ||
it "updates the dependencies" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this case already tested elsewhere for bundler v2? Could we ✂️ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, yes! ✨
a449928
to
a174de3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 think you also need to yank out these specs https://github.com/dependabot/dependabot-core/blob/main/bundler/helpers/v2/spec/functions_spec.rb#L8-L9
2d1fdf8
to
63db7c0
Compare
@@ -1,29 +1,40 @@ | |||
# frozen_string_literal: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like my merge on main created a merge conflict in this file
The python failures seem unrelated, and with that suite taking 60m at the minute (💀), I'm gonna go ahead and merge this. If those tests continue to fail and are not a flake, I'll open a separate PR to fix |
Add native helpers for bundler 2 lockfile updater.
This makes it a bit more clear that we're testing negation.
63db7c0
to
53557ec
Compare
Add native helpers for bundler 2 lockfile updater.