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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate TestServiceUpdatePort #36502

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Remove duplicate TestServiceUpdatePort #36502

merged 1 commit into from
Mar 6, 2018

Conversation

thaJeztah
Copy link
Member

The TestAPIServiceUpdatePort test performs exactly the same steps.

Here's one;

moby/integration-cli/docker_cli_service_update_test.go

Lines 14 to 47 in 5a48410

func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) {
d := s.AddDaemon(c, true, true)
serviceName := "TestServiceUpdatePort"
serviceArgs := append([]string{"service", "create", "--detach", "--no-resolve-image", "--name", serviceName, "-p", "8080:8081", defaultSleepImage}, sleepCommandForDaemonPlatform()...)
// Create a service with a port mapping of 8080:8081.
out, err := d.Cmd(serviceArgs...)
c.Assert(err, checker.IsNil)
waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1)
// Update the service: changed the port mapping from 8080:8081 to 8082:8083.
_, err = d.Cmd("service", "update", "--detach", "--publish-add", "8082:8083", "--publish-rm", "8081", serviceName)
c.Assert(err, checker.IsNil)
// Inspect the service and verify port mapping
expected := []swarm.PortConfig{
{
Protocol: "tcp",
PublishedPort: 8082,
TargetPort: 8083,
PublishMode: "ingress",
},
}
out, err = d.Cmd("service", "inspect", "--format", "{{ json .Spec.EndpointSpec.Ports }}", serviceName)
c.Assert(err, checker.IsNil)
var portConfig []swarm.PortConfig
if err := json.Unmarshal([]byte(out), &portConfig); err != nil {
c.Fatalf("invalid JSON in inspect result: %v (%s)", err, out)
}
c.Assert(portConfig, checker.DeepEquals, expected)
}

And the other;

moby/integration-cli/docker_api_swarm_service_test.go

Lines 32 to 51 in 5a48410

func (s *DockerSwarmSuite) TestAPIServiceUpdatePort(c *check.C) {
d := s.AddDaemon(c, true, true)
// Create a service with a port mapping of 8080:8081.
portConfig := []swarm.PortConfig{{TargetPort: 8081, PublishedPort: 8080}}
serviceID := d.CreateService(c, simpleTestService, setInstances(1), setPortConfig(portConfig))
waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1)
// Update the service: changed the port mapping from 8080:8081 to 8082:8083.
updatedPortConfig := []swarm.PortConfig{{TargetPort: 8083, PublishedPort: 8082}}
remoteService := d.GetService(c, serviceID)
d.UpdateService(c, remoteService, setPortConfig(updatedPortConfig))
// Inspect the service and verify port mapping.
updatedService := d.GetService(c, serviceID)
c.Assert(updatedService.Spec.EndpointSpec, check.NotNil)
c.Assert(len(updatedService.Spec.EndpointSpec.Ports), check.Equals, 1)
c.Assert(updatedService.Spec.EndpointSpec.Ports[0].TargetPort, check.Equals, uint32(8083))
c.Assert(updatedService.Spec.EndpointSpec.Ports[0].PublishedPort, check.Equals, uint32(8082))
}

@thaJeztah
Remove duplicate TestServiceUpdatePort
36e1646 
The TestAPIServiceUpdatePort test performs exactly
the same steps.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Mar 6, 2018
Open
vdemeester
vdemeester approved these changes Mar 6, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

dnephin
dnephin approved these changes Mar 6, 2018
View reviewed changes
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

hm... https://jenkins.dockerproject.org/job/Docker-PRs-experimental/39654/console

15:31:45 === RUN   TestServiceWithPredefinedNetwork
15:31:56 --- FAIL: TestServiceWithPredefinedNetwork (11.33s)
15:31:56 	daemon.go:283: [d016408df2853] waiting for daemon to start
15:31:56 	daemon.go:315: [d016408df2853] daemon started
15:31:56 	service_test.go:52: timeout hit after 10s: task count at 1 waiting for 0
15:31:56 	daemon.go:273: [d016408df2853] exiting daemon
15:31:56 FAIL

Odd, that test was added in #36316, but trying to understand how waiting for 0 can occur

@thaJeztah
Copy link
Member Author

oh! misreading, was looking at service count, not task-count (thanks @vdemeester), so that test is failing at

moby/integration/network/service_test.go

Line 52 in 0b0af85

poll.WaitOn(t, noTasks(client), pollSettings)

yongtang
yongtang approved these changes Mar 6, 2018
View reviewed changes
Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Mar 6, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@f0694e9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36502   +/-   ##
=========================================
  Coverage          ?   34.64%           
=========================================
  Files             ?      613           
  Lines             ?    45404           
  Branches          ?        0           
=========================================
  Hits              ?    15730           
  Misses            ?    27614           
  Partials          ?     2060

@yongtang yongtang merged commit f3a3778 into moby:master Mar 6, 2018
@thaJeztah thaJeztah deleted the remove-duplicate-test branch March 6, 2018 23:24
@thaJeztah thaJeztah mentioned this pull request Apr 17, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 31, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@dnephin dnephin dnephin approved these changes

@yongtang yongtang yongtang approved these changes

Assignees
No one assigned
Labels
area/testing status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thaJeztah @vdemeester @dnephin @yongtang @GordonTheTurtle