libnetwork/iptables: Fix test panic when execute only one test by ch33hau · Pull Request #43384 · moby/moby

ch33hau · 2022-03-15T21:06:37Z

- What I did
The test paniced when executing individually is because they are sharing some same variables (natChain, filterChain, and bridgeName), these variables only initiated on the first test.

- How I did it
This PR moves the shared variables to local variables and initiates them within every test.

- How to verify it
Run a single test:

go test -run TestConcurrencyWithWait github.com/docker/docker/libnetwork/iptables

- Description for the changelog
Fix iptables tests

thaJeztah · 2022-03-15T22:50:50Z

Thank you for contributing! It appears your commit message is missing a DCO sign-off,
causing the DCO check to fail.

We require all commit messages to have a Signed-off-by line with your name
and e-mail (see "Sign your work" in the CONTRIBUTING.md in this repository),
which looks something like:

Signed-off-by: YourFirsName YourLastName <[email protected]>

There is no need to open a new pull request, but to fix this (and make CI pass),
you need to amend the commit(s) in this pull request, and "force push" the amended
commit.

You can find some instructions in the output of the DCO check (which can be found
in the "checks" tab on this pull request), as well as in the Moby contributing guide.

Steps to do so "roughly" come down to:

Set your name and e-mail in git's configuration:
```
git config --global user.name "YourFirstName YourLastName"
git config --global user.email "[email protected]"
```
(Make sure to use your real name (not your GitHub username/handle) and e-mail)
Clone your fork locally
Check out the branch associated with this pull request
Sign-off and amend the existing commit(s)
```
git commit --amend --no-edit --signoff
```
If your pull request contains multiple commits, either squash the commits (if
needed) or sign-off each individual commit.
Force push your branch to GitHub (using the --force or --force-with-lease flags) to update the pull request.

Sorry for the hassle; let me know if you need help or more detailed instructions!

ch33hau · 2022-03-16T09:10:11Z

Hi @thaJeztah sorry for that, I updated.

corhere

Thank you so much for fixing up these tests! I just have one question before approving.

corhere · 2022-11-10T22:48:03Z

+	err = iptable.ProgramChain(natChain, bridgeName, false, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = iptable.ProgramChain(filterChain, bridgeName, false, true)
+	if err != nil {
+		t.Fatal(err)
+	}


How come the chains have to be programmed a second time? Why aren't the ProgramChain calls inside createNewChain sufficient?

Hi @corhere , thanks for reviewing!

Thanks for catching this, I can't see any reasons why they are needed, it should be my mistake. I have removed them.

corhere

LGTM, thanks!

codecov · 2022-11-14T13:18:15Z

Codecov Report

Merging #43384 (3a88a02) into master (ef5b279) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 3a88a02 differs from pull request most recent head a2cea99. Consider uploading reports for the commit a2cea99 to get more accurate results

@@           Coverage Diff            @@
##           master   #43384    +/-   ##
========================================
  Coverage   31.07%   31.08%            
========================================
  Files         642      645     +3     
  Lines       65708    65579   -129     
========================================
- Hits        20421    20383    -38     
+ Misses      43387    43299    -88     
+ Partials     1900     1897     -3

thaJeztah

Left one suggestion; I was writing along, and opened a PR against your branch with my suggestions; ch33hau#1

thaJeztah · 2022-11-14T20:50:50Z

+	iptable, natChain, _, err := createNewChain()
+	if err != nil {
+		t.Fatal(err)
+	}


Perhaps we can make createNewChain() a t.Helper() then we can skip all the "per test" error handling.

Thanks @thaJeztah , that looks a lot cleaner in the code!

- use local variables for chains instead of sharing global variables - make createNewChain a t.Helper Signed-off-by: Chee Hau Lim <[email protected]>

thaJeztah

LGTM!

Thanks so much (was about to ask if you needed help with squashing, but I see you managed to do so ❤️)

thaJeztah added status/2-code-review dco/no Automatically set by a bot when one of the commits lacks proper signature area/networking Networking area/testing labels Mar 15, 2022

ch33hau force-pushed the 42696-panic-test branch from 4d12217 to eddedb8 Compare March 16, 2022 08:59

corhere removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2022

corhere reviewed Nov 10, 2022

View reviewed changes

corhere changed the title ~~Fix test panic when execute only one test~~ libnetwork/iptables: Fix test panic when execute only one test Nov 10, 2022

ch33hau force-pushed the 42696-panic-test branch 2 times, most recently from 6f1d6f3 to 3a88a02 Compare November 14, 2022 02:26

corhere approved these changes Nov 14, 2022

View reviewed changes

thaJeztah mentioned this pull request Nov 14, 2022

libnetwork: make createNewChain a t.Helper ch33hau/moby#1

Merged

thaJeztah reviewed Nov 14, 2022

View reviewed changes

ch33hau force-pushed the 42696-panic-test branch 2 times, most recently from bfd78ed to 1e8e0c6 Compare November 15, 2022 13:06

libnetwork/iptables: Fix test panic when execute only one test

a2cea99

- use local variables for chains instead of sharing global variables - make createNewChain a t.Helper Signed-off-by: Chee Hau Lim <[email protected]>

ch33hau force-pushed the 42696-panic-test branch from 1e8e0c6 to a2cea99 Compare November 15, 2022 13:21

thaJeztah approved these changes Nov 15, 2022

View reviewed changes

corhere merged commit 4651269 into moby:master Nov 17, 2022

thaJeztah added this to the v-next milestone Nov 17, 2022

ch33hau deleted the 42696-panic-test branch November 18, 2022 03:01

thaJeztah mentioned this pull request Dec 31, 2022

[23.0 backport] Check iptables options before looking for ip6tables binary #44727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnetwork/iptables: Fix test panic when execute only one test#43384

libnetwork/iptables: Fix test panic when execute only one test#43384
corhere merged 1 commit into
moby:masterfrom
ch33hau:42696-panic-test

ch33hau commented Mar 15, 2022

Uh oh!

thaJeztah commented Mar 15, 2022

Uh oh!

ch33hau commented Mar 16, 2022

Uh oh!

corhere left a comment

Uh oh!

corhere Nov 10, 2022

Uh oh!

ch33hau Nov 14, 2022

Uh oh!

corhere left a comment

Uh oh!

codecov Bot commented Nov 14, 2022 •

edited

Loading

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Nov 14, 2022

Uh oh!

ch33hau Nov 15, 2022

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ch33hau commented Mar 15, 2022

Uh oh!

thaJeztah commented Mar 15, 2022

Uh oh!

ch33hau commented Mar 16, 2022

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

corhere Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

ch33hau Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

ch33hau Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 14, 2022 •

edited

Loading