Add test for embedded requirements #748
Conversation
|
Ugh okay I think I see the problem, after lifting this out of the other branch there won't be a request for |
jrray
force-pushed
the
embedded-requirements-test
branch
from
65998fd to
62b097f
Compare
|
Okay, now it should be in a state where the test should pass, but it still fails. |
jrray
force-pushed
the
embedded-requirements-test
branch
from
62b097f to
b59cdf5
Compare
Add test to exercise this behavior. Signed-off-by: J Robert Ray <[email protected]>
jrray
force-pushed
the
embedded-requirements-test
branch
from
b59cdf5 to
f00ebb1
Compare
| ] | ||
| ]; | ||
|
|
||
| changes.extend(self.components_to_changes(embedded.components(), embedded.ident())); |
There was a problem hiding this comment.
I found this one addition will make the tests pass. I cribbed this from what resolve_package does, and now I wonder if it would be more correct to do all the things that resolve_package does here too.
There was a problem hiding this comment.
I'm pretty sure it would be more correct, so I went ahead and implemented that.
Codecov Report
@@ Coverage Diff @@
## main #748 +/- ##
==========================================
+ Coverage 53.68% 53.78% +0.10%
==========================================
Files 237 237
Lines 19101 19102 +1
==========================================
+ Hits 10254 10274 +20
+ Misses 8847 8828 -19
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The logic for using `Change::SetPackage` correctly needs to also be used when adding an embedded package to the solution. Signed-off-by: J Robert Ray <[email protected]>
|
I'm not actually sure that requirements on embedded packages was allowed in the past - which might explain why this was not passing... but I'd have to go back and check. Originally, the fields that were allowed for embedded packages was quite limited, but it may have changed over time I don't really remember. Regardless, this change seems appropriate if we want to allow this - though it might be worth thinking about whether it should be allowed if we haven't technically supported it in the past. |
This probably shouldn't be allowed, but it currently works. Signed-off-by: J Robert Ray <[email protected]>
|
An embedded package needs to be able to masquerade as a real package, so it should have all the capabilities of a real package. Where it gets iffy for me is allowing an embedded package to embed packages. |
Oh, the test was failing because it wasn't adding the requirement. Not because the spec parser was rejecting an illegal construct, like how it will reject an embedded package specifying build options. |
|
This came up because I was trying to create a test that would hit but so far I haven't be able to come up with a test that reaches this code. None of the current tests hit this. |
|
#744 effectively disables nested embedded packages because it closes the loophole of being allowed to define them inside an embedded package via |
One of them is. |
Checking solver behavior before the changes in #744. I happened on this while trying to improve the test code coverage of the validators.
@rydrman this test should be passing, yes? I've double and triple checked that the
mypkgdefinition is a valid spec.