Add only those sys.paths which contain an ansible_collections directory path

[audgirka]

Fixes: #323

Depends on #325

[ssbarnea]

I would be far more confident about a test that asserts the fact that a collection that is installed under sitepackages is found after making this change instead of just checking the addition of the folder to the list.

What I do not know is how we can get a collection installed there in order to perform this test.

@cidrblock Any ideas?

The reason I am asking this is because I am not sure if the addition of the path really works and I find behavior test as adding more confidence than one that only does a unit-testing.

[zhan9san]

Plz kindly take a look at this comment
#318 (comment)

[zhan9san]

Do we really need to add isolated in condition here?

#318 (comment)

[cidrblock]

I think this PR fixes #323 but I don't see specifically how it addresses ansible/molecule#4015. Can the title be modified please?

WRT to the change in tests, My preference is to test all permutations of the conditions evalutated in the code.

[cidrblock]

I would be far more confident about a test that asserts the fact that a collection that is installed under sitepackages is found after making this change instead of just checking the addition of the folder to the list.

What I do not know is how we can get a collection installed there in order to perform this test.

@cidrblock Any ideas?

The reason I am asking this is because I am not sure if the addition of the path really works and I find behavior test as adding more confidence than one that only does a unit-testing.

@ssbarnea You are 100% right. The test I added could be improved. The only reason to iterate sys.paths is because it exposes the site-packages directories where a full ansible might be found. I will PR a change to the test shortly.

[cidrblock]

@zhan9san I have captured your concerns as new issues

#323
#324

[audgirka]

I think this PR fixes #323 but I don't see specifically how it addresses ansible/molecule#4015. Can the title be modified please?

WRT to the change in tests, My preference is to test all permutations of the conditions evalutated in the code.

@cidrblock Because the issue states that the collection was installed in the site packages path which was not added to the collection_paths earlier.
Now since we're adding it, this will solve the issue.

[cidrblock]

one comment

[cidrblock]

I would be far more confident about a test that asserts the fact that a collection that is installed under sitepackages is found after making this change instead of just checking the addition of the folder to the list.

What I do not know is how we can get a collection installed there in order to perform this test.

@cidrblock Any ideas?

The reason I am asking this is because I am not sure if the addition of the path really works and I find behavior test as adding more confidence than one that only does a unit-testing.

@ssbarnea I think #325 is a better test, LMKWYT

[zhan9san]

@cidrblock Because the issue states that the collection was installed in the site packages path which was not added to the collection_paths earlier.
Now since we're adding it, this will solve the issue.

I agree with @cidrblock that this PR only fixes the issue described in #323 .

And I think the issue described by @lod in ansible/molecule#4015 (comment) is fixed by #318

But the issue described by @isuftin in ansible/molecule#4015 (comment) is not related to this PR.