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

Clean up temp dir after app tests #9119

Merged
merged 2 commits into from
Nov 25, 2021
Merged

Clean up temp dir after app tests #9119

merged 2 commits into from
Nov 25, 2021

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Nov 24, 2021

There is an intermittent issue where test will fail due to a non-empty
test directory at the end of a test. Temp dirs created by the testing
package are not required to be empty at cleanup time, so something has
gone awry.

After looking at the code, it seems that an some Auth server instances
created as fixtures for several tests are never explicitly closed and
cleaned up when the test finishes. This leads to a hypothesis that
something in the audit writer is writing to the temp dir while it is in
the process of being cleaned up.

This patch add automatic cleanup to the Auth server test fixture, which
executes before the temp directory cleanup. The intention is to stop
any writes to the temp dir at the end of the test, so that it can be
cleaned up without error.

Also marks some not-implemented tests skipped rather than passed, to
increase visibility.

@tcsc
Clean up temp dir after app tests
7342a65 
There is an intermittent issue where test will fail due to a non-empty
test directory at the end of a test. Temp dirs created by the `testing`
package are _not required to be empty_ at cleanup time, so something has
gone awry.

After looking at the code, it seems that an some Auth server instances
created as fixtures for several tests are never explicitly closed and
cleaned up when the test finishes. This leads to a hypothesis that
something in the audit writer is writing to the temp dir _while it is in
the process of being cleaned up_.

This patch add automatic cleanup to the Auth server test fixture, which
executes before the temp directory cleanup. The intention is to stop
any writes to the temp dir at the end of the test, so that it can be
cleaned up without error.

Also marks some not-implemented tests _skipped_ rather than _passed_, to
increase visibility.
codingllama
codingllama approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, those failures are kind of a pain.

lib/srv/app/server_test.go Show resolved Hide resolved
@tcsc tcsc merged commit 49c241f into master Nov 25, 2021
@tcsc tcsc deleted the tcsc/dir-cleanup branch November 25, 2021 00:44
tcsc added a commit that referenced this pull request Dec 1, 2021
@tcsc
Clean up temp dir after app tests (#9119)
b3a58e9 
There is an intermittent issue where test will fail due to a non-empty
test directory at the end of a test. Temp dirs created by the `testing`
package are _not required to be empty_ at cleanup time, so something has
gone awry.

After looking at the code, it seems that an some Auth server instances
created as fixtures for several tests are never explicitly closed and
cleaned up when the test finishes. This leads to a hypothesis that
something in the audit writer is writing to the temp dir _while it is in
the process of being cleaned up_.

This patch add automatic cleanup to the Auth server test fixture, which
executes before the temp directory cleanup. The intention is to stop
any writes to the temp dir at the end of the test, so that it can be
cleaned up without error.

Also marks some not-implemented tests _skipped_ rather than _passed_, to
increase visibility.
@tcsc tcsc mentioned this pull request Dec 1, 2021
Merged
tcsc added a commit that referenced this pull request Dec 1, 2021
@tcsc
Clean up temp dir after app tests (#9119)
3d2cf51 
There is an intermittent issue where test will fail due to a non-empty
test directory at the end of a test. Temp dirs created by the `testing`
package are _not required to be empty_ at cleanup time, so something has
gone awry.

After looking at the code, it seems that an some Auth server instances
created as fixtures for several tests are never explicitly closed and
cleaned up when the test finishes. This leads to a hypothesis that
something in the audit writer is writing to the temp dir _while it is in
the process of being cleaned up_.

This patch add automatic cleanup to the Auth server test fixture, which
executes before the temp directory cleanup. The intention is to stop
any writes to the temp dir at the end of the test, so that it can be
cleaned up without error.
@tcsc tcsc mentioned this pull request Dec 1, 2021
Merged
tcsc added a commit that referenced this pull request Dec 1, 2021
@tcsc
[Backport] Google CloudBuild support (#9090) (#9165)
e8fee61 
Part of this change is implementing a "no secrets" policy for CI. Given that

  1.  we have to support CI for arbitrary external contributors, and
  2.  it is easy to craft a malicious PR that exfiltrates secrets during a CI build

any test that runs under CI must be able to do so without any injected secrets.

This means that several of the test we currently run under Drone will not be 
run on GCB, at least as part of the regular CI. The plan is to create a separate
task that periodically runs tests that require external credentials (e.g. Kube tests,
various backend data stores, etc.) in a more secure way and report failures
asynchronously. And while these tests will not run under CI, the should still be
built under CI so that required changes are caught during review.

Note: this backport includes various data race fixes added separately in the master branch:

See-Also: #8643
See-Also: #8888
See-Also: #9117
See-Also: #9119
tcsc added a commit that referenced this pull request Dec 16, 2021
@tcsc
[Backport] Google CloudBuild support + build fixes (#9090) (#9166)
6509d19 
Part of this change is implementing a "no secrets" policy for CI. Given that

    we have to support CI for arbitrary external contributors, and
    it is easy to craft a malicious PR that exfiltrates secrets during a CI build

any test that runs under CI must be able to do so without any injected secrets.

This means that several of the test we currently run under Drone will not be run
on GCB, at least as part of the regular CI. The plan is to create a separate task
that periodically runs tests that require external credentials (e.g. Kube tests,
various backend data stores, etc.) in a more secure way and report failures
asynchronously. And while these tests will not run under CI, the should still be
built under CI so that required changes are caught during review.

See-Also: #8608
See-Also: #8643
See-Also: #8888
See-Also: #9117
See-Also: #9119
@russjones russjones mentioned this pull request Dec 19, 2021
Closed
@greedy52
Copy link
Contributor

greedy52 commented Dec 21, 2021 •
edited
Loading

I may still see this fails, or is this a different failure @tcsc ?

https://console.cloud.google.com/cloud-build/builds/72e047fc-c18d-49d7-a3ff-41160f376a22;step=0?project=ci-account

testing.go:967: TempDir RemoveAll cleanup: unlinkat /tmp/TestHandleConnection556806069/001/log/upload/streaming/default: directory not empty
--- FAIL: TestHandleConnection (2.91s)
exit status 1
Makefile:462: recipe for target 'test-go' failed

(after some digging. i think just v8 missing this fix: 5e0c8c9)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codingllama codingllama codingllama approved these changes

@russjones russjones russjones approved these changes

@nklaassen nklaassen Awaiting requested review from nklaassen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tcsc @greedy52 @russjones @codingllama