Skip to content

Comments

Move integration/server_test tests to integration-cli#12282

Merged
LK4D4 merged 4 commits intomoby:masterfrom
cpuguy83:remove_some_integration_tests
Apr 14, 2015
Merged

Move integration/server_test tests to integration-cli#12282
LK4D4 merged 4 commits intomoby:masterfrom
cpuguy83:remove_some_integration_tests

Conversation

@cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Apr 10, 2015

No description provided.

@runcom runcom mentioned this pull request Apr 10, 2015
Merged
@crosbymichael
Copy link
Contributor

crosbymichael commented Apr 13, 2015

@cpuguy83 can you please rebase this?

@cpuguy83 cpuguy83 force-pushed the remove_some_integration_tests branch 2 times, most recently from f9d52fb to 2cbdd06 Compare April 13, 2015 17:04
@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 13, 2015

Rebased, failures are due to some bad tests on master after change to sockRequest

@cpuguy83 cpuguy83 force-pushed the remove_some_integration_tests branch 3 times, most recently from 5e8bf03 to 6dd1508 Compare April 13, 2015 18:25
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 13, 2015

@cpuguy83 Haha, guess what :)

@cpuguy83 cpuguy83 force-pushed the remove_some_integration_tests branch from 6dd1508 to f356ebe Compare April 13, 2015 19:00
@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 13, 2015

rebased again.

LK4D4
LK4D4 reviewed Apr 13, 2015
View reviewed changes
integration-cli/docker_cli_create_test.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 13, 2015

Choose a reason for hiding this comment

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

Shouldn't you check that hostname is really set?

Copy link
Member Author

@cpuguy83 cpuguy83 Apr 13, 2015

Choose a reason for hiding this comment

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

The other test was not, certainly can.

Copy link
Contributor

@LK4D4 LK4D4 Apr 13, 2015

Choose a reason for hiding this comment

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

Yeah, just looks weird without it. can be done just with "hostname" command I think.

@cpuguy83 cpuguy83 force-pushed the remove_some_integration_tests branch from f356ebe to 4b3f399 Compare April 13, 2015 19:40
@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 13, 2015

@LK4D4 Updated

@icecrime icecrime removed the dco/yes label Apr 13, 2015
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 13, 2015

@cpuguy83 Fail on test.

@cpuguy83 cpuguy83 force-pushed the remove_some_integration_tests branch from 4b3f399 to 02706a4 Compare April 13, 2015 20:40
@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 13, 2015

Fixed

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 13, 2015

Lol, TestPullVerified

@icecrime icecrime removed the dco/yes label Apr 13, 2015
@icecrime icecrime changed the title Move integration/server_test tests to integreation-cli Move integration/server_test tests to integration-cli Apr 13, 2015
@icecrime
Copy link
Contributor

icecrime commented Apr 13, 2015

TBH I reviewed superficially and haven't looked in details to see if each test did exactly what it used to, but overall LGTM.

ahmetb
ahmetb reviewed Apr 14, 2015
View reviewed changes
integration-cli/docker_api_images_test.go
Copy link
Contributor

@ahmetb ahmetb Apr 14, 2015

Choose a reason for hiding this comment

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

this can be looped as well:

cases := []struct{
    filter string,
    expected int,
  }
  {
    {"utest*/*", 2},
    {"utest", 1},
    // ..and so on
  }

for _, c := range cases {
    if images := getImages(c.filter); len(images[0].RepoTags) != c.expected {
        t.Fatal(errMsg) // <--probably put c.filter here. also good to have something like "expected: %d, got: %d"
    }
}

@ahmetb
Copy link
Contributor

ahmetb commented Apr 14, 2015

LGTM except one suggestion. somebody should really fix that TestPullVerified on windows :D 💃

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 14, 2015

LGTM

LK4D4 added a commit that referenced this pull request Apr 14, 2015
@LK4D4
Merge pull request #12282 from cpuguy83/remove_some_integration_tests
676bf4a 
Move integration/server_test tests to integration-cli
@LK4D4 LK4D4 merged commit 676bf4a into moby:master Apr 14, 2015
@cpuguy83 cpuguy83 deleted the remove_some_integration_tests branch April 14, 2015 19:21
@cpuguy83 cpuguy83 mentioned this pull request Apr 16, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cpuguy83 @crosbymichael @LK4D4 @icecrime @ahmetb @jessfraz @GordonTheTurtle