Snapcraft

Code review comment for lp:~zyga/snapcraft/plainbox-app

plainbox-app
Merge into core

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2015-09-30:

I'm not aware of any issues with $PLAINBOX_PROVIDER_DATA. I'll rebase
this (there's a 2nd patch I didn't push that's a pain to touch as I
changed the IDs of each job) and see if I can reproduce it.

I get your point for other comments. I'll iterate on this. Thanks for
the feedback. :-)

On Wed, Sep 30, 2015 at 3:55 PM, Federico Gimenez
<email address hidden> wrote:
>> Test plans is that something that the current script ran, we can
>> default to normal and switch to --examples with an option (it's
>> trivial as you see).
>
> There's another thing here, for running the examples test plan we had to make some adjustments in the runtests.sh script [1], because plainbox was not able to reach the files out of $PLAINBOX_PROVIDER_DATA. Are you aware of that, will this patch take that into account?
>
> [1]http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/integration-tests/runtests.sh#L37
>
>> If we integrate unit tests into test plans then
>> it would just be a selection of a test plan that matters (and we could
>> have a .* test plan that runs everything).
>>
>
> Yes, that would be great
>
>> What kind of verbose output would you like to have, to show live
>> output as we run (not only when we fail?)
>>
>
> I think that it would be useful for debugging to have the same output as we do now with the plainbox run call used in runtests.sh, which shows all the commands executed.
>
> Also there are some changes required for autopkgtest to work, the new dependencies should be added in [2] (including required version numbers if needed) and the calls at [3] and [4] should be adapted to the new script.
>
> [2] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/control
> [3] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/runexamples
> [4] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/runtests
>
> Thanks!
>
> --
> https://code.launchpad.net/~zyga/snapcraft/plainbox-app/+merge/272720
> You are the owner of lp:~zyga/snapcraft/plainbox-app.

I'm not aware of any issues with $PLAINBOX_PROVIDER_DATA. I'll rebase
this (there's a 2nd patch I didn't push that's a pain to touch as I
changed the IDs of each job) and see if I can reproduce it.

I get your point for other comments. I'll iterate on this. Thanks for
the feedback. :-)

On Wed, Sep 30, 2015 at 3:55 PM, Federico Gimenez
<fgimenez@canonical.com> wrote:
>> Test plans is that something that the current script ran, we can
>> default to normal and switch to --examples with an option (it's
>> trivial as you see).
>
> There's another thing here, for running the examples test plan we had to make some adjustments in the runtests.sh script [1], because plainbox was not able to reach the files out of $PLAINBOX_PROVIDER_DATA. Are you aware of that, will this patch take that into account?
>
> [1]http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/integration-tests/runtests.sh#L37
>
>> If we integrate unit tests into test plans then
>> it would just be a selection of a test plan that matters (and we could
>> have a .* test plan that runs everything).
>>
>
> Yes, that would be great
>
>> What kind of verbose output would you like to have, to show live
>> output as we run (not only when we fail?)
>>
>
> I think that it would be useful for debugging to have the same output as we do now with the plainbox run call used in runtests.sh, which shows all the commands executed.
>
> Also there are some changes required for autopkgtest to work, the new dependencies should be added in [2] (including required version numbers if needed) and the calls at [3] and [4] should be adapted to the new script.
>
> [2] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/control
> [3] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/runexamples
> [4] http://bazaar.launchpad.net/~snappy-dev/snapcraft/core/view/head:/debian/tests/runtests
>
> Thanks!
>
> --
> https://code.launchpad.net/~zyga/snapcraft/plainbox-app/+merge/272720
> You are the owner of lp:~zyga/snapcraft/plainbox-app.

« Back to merge proposal