Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: set up the spread execution in linode #1374

Merged
merged 9 commits into from Jun 23, 2017
Merged

Conversation

come-maiz
Copy link
Contributor

No description provided.

@come-maiz
Copy link
Contributor Author

@fgimenez can you please review this?

Copy link

@fgimenez fgimenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple of minor comments

linode:
key: "$(HOST: echo $SPREAD_LINODE_KEY)"
systems:
- ubuntu-16.04-64:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default linode kernels don't play well with snapd because of the lack of apparmor support, not sure if this apply here but, because you install snapd later, maybe you should use a specific kernel like we do in snapd's suite https://github.com/snapcore/snapd/blob/master/spread.yaml#L51

spread.yaml Outdated
key: "$(HOST: echo $SPREAD_LINODE_KEY)"
systems:
- ubuntu-16.04-64:
workers: 3

suites:
spread_tests/:
summary: tests of the snapcraft snap
prepare: |
apt update

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been suffering lots of problems regarding apt and connection issues from linode hosts to ubuntu mirrors over ipv6 (which is enabled by default), if you hit those, in the form of tests timing out, you could either disable ipv6 or give it a low priority (the latter is giving very good results to us so far https://github.com/snapcore/snapd/blob/master/spread.yaml#L294)

@come-maiz
Copy link
Contributor Author

Thanks @fgimenez. I've copied your bits.

@@ -5,5 +5,6 @@ execute: |
echo "Check that we are using the snapcraft snap"
snapcraft_path="$(which snapcraft)"
[ "$snapcraft_path" = "/snap/bin/snapcraft" ]
source /snapcraft/venv/bin/activate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary given that it's already in the suite's prepare step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, apparently it happens in a different environment or something. I added it just because without it, it failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, okay. Does it need to be in the suite's prepare step, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I merged before reading this question.
Yes, how would I install the requirements in the venv without activating it?

source /snapcraft/venv/bin/activate
pip install --upgrade pip
pip install -r /snapcraft/requirements.txt -r /snapcraft/requirements-devel.txt
apt install --yes snapd
snap install snapcraft --classic --edge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... I don't understand what's happening here. You're installing via pip, but also installing the snap. They'll both be in the path. Which one is being tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm installing only the requirements. They are all needed to run the integration tests. I didn't add the ., and take a look here where we chec that we are using the right command:
https://github.com/snapcore/snapcraft/pull/1374/files#diff-408b0efbb4ace174e50d6bd885ae24baL7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, stupid periods. So easy to miss. Thanks for the info!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!!!

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@come-maiz come-maiz merged commit a34bb1c into master Jun 23, 2017
@come-maiz come-maiz deleted the spread_linode branch June 23, 2017 16:52
@kyrofa kyrofa added this to the 2.32 milestone Jun 23, 2017
kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants