Do not freeze file-based Poetry dependency version

[phillipuniverse]

Fixes #4333. This was modeled after #2361 with the same type of tests.

I also renamed the test files to differentiate between directory-based and file-based dependencies, both of which can be specified by a path.

[phillipuniverse]

I don't really write Ruby but maybe this is better written as:

next if ["directory", "file"].include? locked_details&.dig("source", "type")

Also, looking at the schema we might need to also special-case URL dependencies.

[jurre]

I slightly prefer it, but I don't feel strongly about it

[jurre]

Rubocop will complain that this is indented 4 spaces instead of 2

[jurre]

I noticed a couple of files names were changed but their references in the tests weren't, those tests will likely fail because of that I think?

Overall I think this looks good, but it's been a while since I looked at this code so might need to dig into it a bit more.

[phillipuniverse]

@jurre thanks for the feedback, changes applied!

I'm still not 100% sure if we need to special-case url pieces or not. It looks like there are already tests for URL dependencies. There's already a test for them so maybe this is already doing the right thing? I'm just not sure how to validate the runtime behavior.

dependabot-core/python/spec/dependabot/python/file_parser/poetry_files_parser_spec.rb

Lines 174 to 181 in 35032ea

	context "with a url dependency" do
	let(:pyproject_fixture_name) { "url_dependency.toml" }
	let(:pyproject_lock_fixture_name) { "url_dependency.lock" }
	it "excludes the url dependency" do
	expect(dependencies.map(&:name)).to_not include("toml")
	end
	end

[geori]

had a typo when ya'll dropped the quotes

[phillipuniverse]

@jurre sorry for so much back and forth, TBH I'm writing this a little blind. I'll actually set up a dev environment for this if it fails after the next run.

2 follow-up questions:

1. Do you think we need to do anything about the url dependencies or do you think those will be covered?
2. What's the process/timeline for this fix being included in the official GitHub Dependabot?

Thanks for your help!

[jurre]

Do you think we need to do anything about the url dependencies or do you think those will be covered?

Tbh, I am not sure! Maybe we can add a test for it?

What's the process/timeline for this fix being included in the official GitHub Dependabot?

Once it's merged I can cut a release fairly quickly and can roll it out usually the same or next day.

[jurre]

There's a couple of failing python tests on CI @phillipuniverse, would you mind taking a look at those?

[phillipuniverse]

@jurre yup, sorry for letting this languish I'll get this sorted out today.

[phillipuniverse]

ok! @jurre I feel pretty good about this now, sorry again for how long this whole thing has taken.

New updates:

• Rebased on top of latest updates on main
• URL dependencies had the same problem and were incorrectly adding a version attribute, I added a test and whitelisted it
• Added more positive assertions for directory and file dependencies -- essentially make sure the dependency is actually there but the dependency + version is not

Thanks for your help getting this over the line! We should probably squash merge this when it's all done.

[phillipuniverse]

@jurre last thing here - I have 1 failing test locally and I'm not sure why:

But even with my change in python/lib/dependabot/python/file_updater/pyproject_preparer.rb commented out it still fails. And I even checked out main and ran it and still fails so... 🤷 hopefully it won't fail in CI!

[jurre]

I have 1 failing test

I am currently working on upgrading pipenv so I think I can tackle that failure, it doesn't seem related to these changes

[jurre]

Thanks!