Skip to content

Add test for github.com special handling - #390

Merged
vdemeester merged 1 commit into
docker:masterfrom
thaJeztah:add-test-for-github-special-case
Aug 14, 2017
Merged

Add test for github.com special handling#390
vdemeester merged 1 commit into
docker:masterfrom
thaJeztah:add-test-for-github-special-case

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

Found this code still lingering around on my computer 😄

- Description for the changelog

Add test for github.com special handling

- A picture of a cute animal (not mandatory but encouraged)
pika-pixdaus

(image taken from https://sustainableutah.files.wordpress.com/2015/03/pika-pixdaus.jpg)

Comment threadcli/command/image/build_test.go Outdated
os.Chdir(tmpDir)
os.MkdirAll(filepath.Join(tmpDir, "github.com/docker/no-such-repository"), 0777)
err = cmd.Execute()
assert.Contains(t, err.Error(), "unable to prepare context: unable to evaluate symlinks in Dockerfile path:")

@dnephindnephinJul 27, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the expectation here is "does build from local directory" maybe this would be more clear by adding a simple Dockerfile to the directory and checking for no error?

This error could easily change and fail the test, but that doesn't necessary mean the test is broken.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agreed, I'll look into that

Comment threadcli/command/image/build_test.go Outdated
// TestRunBuildFromLocalGitHubDirNonExistingRepo tests that build contexts
// starting with `github.com/` are special-cased, and the build command attempts
// to clone the remote repo, however a local directory should take precedence.
func TestRunBuildFromLocalGitHubDirNonExistingRepo(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be split into two test cases

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Was doubting; in a single test, it more "proves" that without local directory it does X, and with it does Y, but guess it doesn't matter if split; will update

@thaJeztah
thaJeztahforce-pushed the add-test-for-github-special-case branch from 586d77c to 1d5cdf9CompareAugust 2, 2017 15:26
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the add-test-for-github-special-case branch from 1d5cdf9 to f007d62CompareAugust 3, 2017 15:53
@codecov-io

codecov-io commented Aug 3, 2017

Copy link
Copy Markdown

Codecov Report

Merging #390 into master will increase coverage by 0.34%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #390 +/- ##
==========================================
+ Coverage 46.26% 46.61% +0.34% 
==========================================
Files 193 193 Lines 16093 16093 ==========================================
+ Hits 7445 7501 +56 + Misses 8259 8203 -56 
Partials 389 389

@dnephindnephin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

ping @vdemeester PTAL 😅

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🐸
@thaJeztah one thing we should try to handle though is that, as of now, those tests might fail if there is no network (as it might fail with a different error)

@vdemeester
vdemeester merged commit aaf6939 into docker:masterAug 14, 2017
@GordonTheTurtleGordonTheTurtle added this to the 17.08.0 milestone Aug 14, 2017
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@vdemeester good call; do we have a requires(network) or similar constraint for tests?

@thaJeztah
thaJeztah deleted the add-test-for-github-special-case branch August 14, 2017 09:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah@codecov-io@vdemeester@dnephin@GordonTheTurtle