refactor: use craft-application 5 #856
Conversation
This moves unit tests that were previously being done on the application into the ProjectService which actually does it.
b4fc703 to
50c24df
Compare
1547a9c to
45b17b2
Compare
A restore script that fails to run has the side effect of killing the entire worker, meaning that a whole bunch of tests get aborted.
This is temporary while we fix the issue related to the build plan rejecting the ':' notation.
|
|
||
| @pytest.fixture | ||
| def project_keys(request: pytest.FixtureRequest) -> dict[str, Any]: | ||
| """Fixture to modify the default project as needed. |
There was a problem hiding this comment.
Hol'up really?
I ask because when @bepri and I were writing it I thought you would have the most/best ideas for improvements.
Context: I really want to add a fixture like this to craft-application's pytest plugin so we can have a uniform way to generate projects, and this was a first attempt. (The craft-application one would come with some serious docs about setting up your app's tests)
There was a problem hiding this comment.
I'm totally game for doing a craft-app design of such a fixture, but this is plenty good for what we need now (which is to merge this PR)
These tests manipulate the docker snap itself, so let's run them last so that they don't leave a broken system for the next tests.
The test itself just needs to build, it doesn't matter whether it's priming or packing - and there's currently an unrelated bug with 'prime'.
Co-authored-by: Tiago Nobrega <tiago.nobrega@canonical.com>
The schema is now stricter than what the application supports for `platform` entries.
There is something wrong with the test that needs to be investigated: it times out when packing the rock, and then the 'restore' script times out when cleaning the lxd instance.
| build_plan = self._services.get("build_plan").plan() | ||
|
|
||
| if len(self._build_plan) > 1: | ||
| if len(build_plan) > 1: |
There was a problem hiding this comment.
minor consistency issue, but above you do != 1 (https://github.com/canonical/rockcraft/pull/856/files#diff-1c50437519bc7c33b649e77b5197022dadb1baad316acc13d1822c1a7cb6f327R72)
I am not certain about the behavior of len being 0 here though, for which this raised error would be incorrect
There was a problem hiding this comment.
I'll change the other one to be > 1 as it's more semantically clear what we're checking and why, imo.
There was a problem hiding this comment.
do that in a separate PR later please
no more work on this unless it's absolutely necessary
There was a problem hiding this comment.
Oh, alright, I'll leave it be then
| if len(self._build_plan) != 1: | ||
| build_plan = self._services.get("build_plan").plan() | ||
|
|
||
| if len(build_plan) != 1: |
There was a problem hiding this comment.
1 for the raised error to make sense, not sure what happens when this is 0
There was a problem hiding this comment.
It looks like upstream will raise an error for us if there's no plans:
https://github.com/canonical/craft-application/blob/main/craft_application/services/buildplan.py#L63
make lint && make test?