tests: share the cache dir in the integration suite #1530

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Member

elopio commented Sep 5, 2017

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

Member

elopio commented Sep 6, 2017

With something like this, we could share the cache between travis runs:
http://paste.ubuntu.com/25475894/

However, reading the docs, that doesn't sound that will make things faster. It stores the cache in s3, so it will have to download it and upload it on every test. We don't yet have any pruning of the cache, so it will just grow with old data. And the download/upload will happen even for test stages that don't use it at all.

Member

elopio commented Sep 6, 2017

@kyrofa making some bad statistics looking only at a few samples, it seems we save like 4 minutes only. Probably, because the downloads in travis are super fast. It would be nicer to time the tests, and look individually at the slowest ones.

+
+ # share the cache in all tests.
+ self.useFixture(fixtures.EnvironmentVariable(
+ 'XDG_CACHE_HOME', BaseDirectory.xdg_cache_home))
@kalikiana

kalikiana Sep 6, 2017

Collaborator

It strikes me as wrong to use the actual cache home since it could hide bugs related to downloading missing packages. I'd expect a temporary folder to be used for all tests.

@sergiusens

sergiusens Sep 6, 2017

Collaborator

The realization on my side is, the unit tests are fine with isolation. The integration tests and demos are the ones we need to fix excepting the integration tests that actually test the cache.

Member

kyrofa commented Sep 6, 2017

However, reading the docs, that doesn't sound that will make things faster. It stores the cache in s3, so it will have to download it and upload it on every test. We don't yet have any pruning of the cache, so it will just grow with old data. And the download/upload will happen even for test stages that don't use it at all.

Yeah, I use features like that in gitlab-ci. It works for me though because it caches it locally instead of pushing to S3. As soon as it goes offsite I think it makes less sense, at least in the data-heavy way we're using it.

[...] making some bad statistics looking only at a few samples, it seems we save like 4 minutes only. Probably, because the downloads in travis are super fast. It would be nicer to time the tests, and look individually at the slowest ones.

Actually that makes sense right now if you assume they mirror the Ubuntu archive. But integration tests don't include any ROS tests right now-- do this for the snaps tests, or wait until a few of my upcoming PRs land introducing integration tests using ROS, and I bet it'll help.

@@ -208,6 +208,8 @@ def _setup_origin(self, snapcraft_files, repo_dir, base_dir):
]
def test_local_wiki(self):
+ self.useFixture(fixtures.EnvironmentVariable(
+ 'XDG_CACHE_HOME', os.path.join(self.path, '.cache')))
@kyrofa

kyrofa Sep 6, 2017

Member

I'm curious why we don't want the global cache here? Is it actually testing the cache itself?

@elopio

elopio Sep 6, 2017

Member

The parser is using the cache as a temporary directory, I don't remember why, but here's a TODO related to this:

BASE_DIR = os.path.join(BaseDirectory.xdg_cache_home, 'snapcraft-parser')

So, when we use the real cache, we are affecting these tests in mysterious ways. To fix this, we need to finish that TODO.

@kyrofa

kyrofa Sep 12, 2017

Member

Huh, okay, fair enough.

@sergiusens

sergiusens Nov 7, 2017

Collaborator

This is where the wiki data gets saved to, overwriting it with data from mocked/fake servers would not be great

Member

kyrofa commented Sep 6, 2017

As soon as it goes offsite I think it makes less sense, at least in the data-heavy way we're using it.

I may eat my words once ROS is in there, though. Keep that paste in your back pocket, I bet S3 is faster than the ROS archive.

kyrofa approved these changes Sep 6, 2017

I have one clarifying question, but overall this is wonderful, thank you 😄 .

Member

elopio commented Sep 14, 2017

@kalikiana are you -1 on this branch?

Collaborator

kalikiana commented Sep 18, 2017

@elopio What's your stance on my concern with new/ old packages hiding bugs? And, did you do any timing to see if this is worth it? EDIT: I mean, did you do more complete timing, afaiu you only checked a few individual ones and they weren't great. I could be convinced.

Member

elopio commented Sep 19, 2017

@kalikiana I am undecided about the cache hiding bugs. What about controlling this with an env var like SNAPCRAFT_TEST_SHARE_CACHE, and enable it for pull requests, where we need speed. But disable it for tests on master and the nightly?

The only timing I did was looking at the full suite time reported by travis, and indeed, it wasn't a big improvement. But, the slow ros tests are coming, and that's why started discussing this PR.

Collaborator

kalikiana commented Sep 21, 2017

@elopio Env var sounds like a great idea. That would relieve my concerns.

"Not a big improvement" sounds a bit negative... but if add the env var I'm okay with it either way.

Member

elopio commented Sep 22, 2017

@kalikiana my hope is that if our caching improves, the build times will improve too.

@elopio elopio closed this Sep 22, 2017

@elopio elopio reopened this Sep 22, 2017

Member

kyrofa commented Oct 2, 2017

Where does this PR stand?

Member

elopio commented Oct 6, 2017

I'm inclined to close it. But I'm also wondering about our caching, I was expecting more.

@sergiusens sergiusens added this to the 2.36 milestone Oct 26, 2017

@kyrofa kyrofa referenced this pull request Nov 6, 2017

Merged

catkin plugin: check for pip packages in part only #1717

6 of 6 tasks complete

@sergiusens sergiusens modified the milestones: 2.36, 2.35 Nov 7, 2017

Collaborator

sergiusens commented Nov 9, 2017

@elopio Can you make this shared cache thing applied only to the ros test class?

Member

elopio commented Nov 9, 2017

I couldn't make sense of that ROS-only request on top of this branch. So, what do you think about #1725 ?

@sergiusens sergiusens removed this from the 2.35 milestone Nov 10, 2017

@sergiusens sergiusens closed this Nov 10, 2017

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