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.

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

Several e2e fixes #18283

Merged
merged 8 commits into from
Apr 20, 2023
Merged

Several e2e fixes #18283

merged 8 commits into from
Apr 20, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Apr 20, 2023

Split out some fixes from #18163 that can be merged without ginkgo v2.
this should make review easier. Please see the individual commits for details.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 20, 2023
session = podmanTest.Podman([]string{"run", "--rm", "-v", ".:/app:O", ALPINE, "ls", "/app"})
session.WaitWithDefaultTimeout()
Expect(session.OutputToString()).To(ContainSubstring("hello"))
Expect(session.OutputToString()).To(ContainSubstring("run_volume_test.go"))
Copy link
Member

Choose a reason for hiding this comment

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

super-duper-minor nit: instead of hardcoding, might it be possible to use Caller()? Again, super-minor, it seems impossible for this to fail: even if the source file is renamed, the error would be caught immediately. It's just one of my neuroses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhh searching around there is CurrentGinkgoTestDescription().FileName, I wasn't ware of such functionality before.
If I have to repush I can add it.

@edsantiago
Copy link
Member

Lots of really nice catches. Thanks for breaking them out into a separate PR with separate commits.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@edsantiago
Copy link
Member

Sigh.... needs to wait for #18282 which is currently stuck in CI-flake land.

@Luap99
Copy link
Member Author

Luap99 commented Apr 20, 2023

Test are failing because of #18282, I will rebase once this merges and apply the nit from @edsantiago

Luap99 added 8 commits April 20, 2023 16:26
Because the test mounts the current dir it does not need to create a new
file in it. Just check if the current test file is there should fulfill
the same purpose.

Signed-off-by: Paul Holzinger <[email protected]>
Running these locally always created a popup to ask me for my password
as I am in the wheel group.
I would also argue that such a test should not be run on any local
system ever even as root. First docker could be a symlink to podman so
the check if the image is there would fail. Second starting the docker
deamon in a podman test suite just feels very unexpected.

Signed-off-by: Paul Holzinger <[email protected]>
If a unit is not active the exit code from systemctl is 3. Thus this
test always failed because it checked the error.

Fix this by checking the exit code and remove the unnecessary output
parsing.

Signed-off-by: Paul Holzinger <[email protected]>
Some network test use the same subnet as others, because the network
config direcory is shared we must ensure subnets do not conflict as
tests are run in parallel. I see this locally when running with 12
threads.

Signed-off-by: Paul Holzinger <[email protected]>
Remove the file from the cwd after the test.

Signed-off-by: Paul Holzinger <[email protected]>
Creating a new diretory results in the test leaking it when it is not
removed via a defer call. All tests have already access to
`podmanTest.TempDir` which will be automatically removed in the
`AfterEach()` block.

While some test were fine other forgot the defer call. To keep the test
consitent and prevent other from making the same mistake convert all
users to `podmanTest.TempDir`. `CreateTempDirInTempDir()` is only used
for the `podmanTest.Setup()` call.

Signed-off-by: Paul Holzinger <[email protected]>
This is a rather big deal. All system services shared the same tmpdir
which causes big issues for the rootless netns setup.
Also use --events-backend file like the local ones. This is important
otherwise reading events and takes ages as the jounal is shared for all
tests.

Signed-off-by: Paul Holzinger <[email protected]>
When running the remote integration test I have over 1000 zombies
because each test creates a single service process. Only after ginkgo
exists they get finally reaped by the init process. This only effected
the rootless runs.

For some reason the test use different logic between root and rootless.
This doesn't make much sense. I also see no reason to manually kill
child processes.

Signed-off-by: Paul Holzinger <[email protected]>
@edsantiago
Copy link
Member

LGTM and tests are green. @containers/podman-maintainers PTAL.

@rhatdan
Copy link
Member

rhatdan commented Apr 20, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2023
@mheon
Copy link
Member

mheon commented Apr 20, 2023

LGTM. I'm just going to press the button in case the bot has not recovered yet.

@mheon mheon merged commit 147f198 into containers:main Apr 20, 2023
@Luap99 Luap99 deleted the e2e-fixes branch April 21, 2023 09:12
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants