VCS tools are test dependencies… #427

Merged
merged 1 commit into from Apr 8, 2016

Conversation

Projects
None yet
4 participants
Contributor

vilagithub commented Apr 5, 2016

...they should be installed to run the tests or the tests should be skipped.

This allows tests to be run concurrently (apt-get can't be run concurrently
;) and brings the time to run both unit and integration tests from 5'47 to 1'43 with:

PYTHONPATH=pwd ols-run-tests -m integration_tests -m snapcraft -c 8

which is nice ;)

No tests were hurt during the production of this patch ;)

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Member

elopio commented Apr 5, 2016

OK to test.

Collaborator

sergiusens commented Apr 5, 2016

this is missing a bug number

Member

elopio commented Apr 5, 2016

lgtm. Maybe we can add in runtests.sh a check for all the requirements and fail if they are missng so we can also run the tests with that script, not just with adt-run.

Member

elopio commented Apr 5, 2016

also note that some plugins will execute apt-get to install their build packages. So this is good, but doesn't remove all the executions of apt-get in the code.
A nice improvement would be to have retries on snapcraft/repo.py in case somebody else has the apt lock.

Contributor

vilagithub commented Apr 6, 2016

this is missing a bug number

Right, I wasn't sure how to phrase it nor if it was worth fixing so I thought about gathering feedback with that PR first ;)

Contributor

vilagithub commented Apr 6, 2016

note that some plugins will execute apt-get to install their build packages. So this is good, but doesn't remove all the executions of apt-get in the code.

Right, this was focused on tests only and only on tests that broke isolation and block concurrency.

A nice improvement would be to have retries on snapcraft/repo.py in case somebody else has the apt lock.

I'm not sure I follow. As a user I don't expect to get meaningful results by running apt-get install concurrently (even through different commands).

For tests though, I would argue that if 'sudo apt-get install' needs to be used in a test this should happen in some vm where passwordless sudo access is provided.

Developers running tests are unlikely to provide passwordless sudo access and some would (rightly) freak out if running tests prompt for the root password ;)

I do my snapcraft work inside a container where passwordless sudo access is provided but I don't think I'm in the majority there...

Contributor

vilagithub commented Apr 6, 2016

Found https://bugs.launchpad.net/snapcraft/+bug/1540523 which looks like a good match.
@sergiusens, do you agree ?

Collaborator

sergiusens commented Apr 6, 2016

El 06/04/16 a las 10:59, vilagithub escribió:

Found https://bugs.launchpad.net/snapcraft/+bug/1540523 which looks like
a good match.
@sergiusens https://github.com/sergiusens, do you agree ?

Could be, that bug is much broader though (implementing requirements.txt and requirements-test.txt for dependencies as well).

Contributor

vilagithub commented Apr 6, 2016

Could be, that bug is much broader though (implementing requirements.txt and requirements-test.txt for dependencies as well).

Ha right, yeah, will file a more focused one then.

Contributor

vilagithub commented Apr 7, 2016

Collaborator

sergiusens commented Apr 8, 2016

you will need to click on "Update branch".

Ping one of us when you do so we know not to land anything in between.

Thanks

VCS tools are test dependencies, they should be installed to run the …
…tests or the tests should be skipped.

This allows tests to be run concurrently (apt-get can't be run concurrently)
and brings the time to run the integration tests from 4'14 to 1'37 with:

  PYTHONPATH=`pwd` ols-run-tests -m integration_tests -c 8

Combined with the unit tests, this bring the time to run both unit and
integration tests from 5'47 to 1'43 with:

  PYTHONPATH=`pwd` ols-run-tests -m integration_tests -m snapcraft -c 8

which is nice ;)

No tests were hurt during the production of this patch ;)

LP: #1566882
Contributor

vilagithub commented Apr 8, 2016

From http://162.213.35.179:8080/job/github-snapcraft-autopkgtest-cloud/429/console :

Finding IP address succeeded: 10.42.61.202
Waiting until ssh becomes available
Timed out waiting for ssh. Aborting! Console log:

is probably the relevant error: autopkgtest fails to setup the slave

Contributor

vilagithub commented Apr 8, 2016

Rebased and test passing \o/

@sergiusens sergiusens merged commit 38b95b1 into snapcore:master Apr 8, 2016

4 checks passed

Examples tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.67%
Details

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

VCS tools are test dependencies, they should be installed to run the …
…tests or the tests should be skipped. (#427)

This allows tests to be run concurrently (apt-get can't be run concurrently) and brings the time to run the integration tests from 4'14 to 1'37 with:

    PYTHONPATH=`pwd` ols-run-tests -m integration_tests -c 8

Combined with the unit tests, this bring the time to run both unit and integration tests from 5'47 to 1'43 with:

    PYTHONPATH=`pwd` ols-run-tests -m integration_tests -m snapcraft -c 8

which is nice ;)

No tests were hurt during the production of this patch ;)

LP: #1566882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment