Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Store version map in cache on success (instead of failure) to retrieve it. #514

Merged
merged 1 commit into from
May 5, 2017

Conversation

ChrisHines
Copy link
Contributor

Fixes #144.

I'm not sure how to add a test for this since the tests for sourceGateway assume git.

@sdboyer
Copy link
Member

sdboyer commented May 4, 2017

roflcopter 🤣 🤣 🤣 🤣 🤣

good catch, thank you 😄

so, for tests...yeah. this certainly does highlight why we'd want to test more than just the git source gateway - this specific behavior is one that differs because of how the different maybeSource impls work for different vcses, and now hey look! it's the one we missed.

i think the easiest (relatively speaking) thing to do for tests would be:

  1. rename testSourceGateway to testGitSourceGateway
  2. just copy the whole func body into a new testHgSourceGateway
  3. adapt values from testHgSourceInteractions into the new test

hg and bzr work the same way with respect to this particular bug, so i think just adding the one is should be adequate for now.

@ChrisHines
Copy link
Contributor Author

Are there a common set of behavioral invariants across all implementations that we could test for? I'm at a disadvantage being new to the code base.

@sdboyer
Copy link
Member

sdboyer commented May 4, 2017

Between git, bzr, and hg, this is - unless I'm forgetting something - the only behavioral difference at the level of the sourceGateway. The difference is a product of relying on git ls-remote as the "ping" mechanism to determine an upstream repository's existence - it would be very wasteful (another network request later) to ask for a version list and discard it. bzr and hg, OTOH, separate pinging from listing versions.

svn is something else, but we really don't even properly support that yet, so best to leave it out of discussion.

sourceGateway itself is a slapdash FSM, with require() being the mechanism for effecting transitions. The rigorous-but-brute-force approach to testing it would probably be enumerating the possible transition space (it's not TOO huge) and testing each. That, or a smarter reduction of it, would be nice to have, but is a task for another PR that doesn't have a crucial bugfix 😄. The contents of testSourceGateway is my basic attempt at covering most of the transitions, which is why I think duplicating it for hg would be adequate.

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

This is actually causing larger issues than I'd realized - I misdiagnosed a different problem as this one last week. Do you think you'll have time to tackle additional tests anytime soon? If not, I understand, but I'd like to merge this and we can do more tests in a follow-up.

@ChrisHines
Copy link
Contributor Author

I might have time to work on adding tests this weekend, but can't promise I'll have them working in the next few days. If you merge this before then I'll just open a new PR to add the tests.

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

@ChrisHines great, I'll merge now then. thank you!

@sdboyer sdboyer merged commit 15e7e79 into golang:master May 5, 2017
ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017
Store version map in cache on success (instead of failure) to retrieve it.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants