internal/driver: skip TestHttpsInsecure test on iOS #256
Conversation
Codecov Report
@@ Coverage Diff @@
## master #256 +/- ##
=======================================
Coverage 65.38% 65.38%
=======================================
Files 34 34
Lines 7173 7173
=======================================
Hits 4690 4690
Misses 2088 2088
Partials 395 395
Continue to review full report at Codecov.
|
|
I haven't contributed to this repository before, but fwiw the Travis failures doesn't seem related to this PR. |
internal/driver/fetch_test.go
Outdated
| // CPU profiling is not supported on Plan9; see golang.org/issues/22564. | ||
| return | ||
| case "darwin": | ||
| if runtime.GOARCH == "arm" || runtime.GOARCH == "arm64" { | ||
| // CPU profiling is not supported on OS; see golang.org/issues/22612. |
There was a problem hiding this comment.
Is CPU profiling not supported on iOS in the same way as on plan9, or the quality of profiles is low and there are no symbolized function names in the profile? Looking at https://github.com/golang/go/blob/83a1a2ba63cd79ed65a97bead2c31ef0753f4cd2/src/runtime/pprof/pprof_test.go#L148, I don't see the CPU profile test being skipped in the Go tests?
There was a problem hiding this comment.
I think CPU profiling works on iOS, so my guess would be missing symbols, judging from the failure log:
Local symbolization failed for cppbench_server_main: stat cppbench_server_main: no such file or directory
Local symbolization failed for libpthread-2.15.so: stat /libpthread-2.15.so: no such file or directory
Some binary filenames not available. Symbolization may be incomplete.
Try setting PPROF_BINARY_PATH to the search path for local binaries.
Local symbolization failed for cppbench_server_main: stat cppbench_server_main: no such file or directory
Local symbolization failed for libpthread-2.15.so: stat /libpthread-2.15.so: no such file or directory
Some binary filenames not available. Symbolization may be incomplete.
Try setting PPROF_BINARY_PATH to the search path for local binaries.
Local symbolization failed for cppbench_server_main: stat cppbench_server_main: no such file or directory
Local symbolization failed for libpthread-2.15.so: stat /libpthread-2.15.so: no such file or directory
Some binary filenames not available. Symbolization may be incomplete.
Try setting PPROF_BINARY_PATH to the search path for local binaries.
I updated the comment to reflect that.
internal/driver/fetch.go
Outdated
| @@ -294,7 +294,7 @@ func setTmpDir(ui plugin.UI) (string, error) { | |||
| dirs = append(dirs, os.TempDir()) | |||
| for _, tmpDir := range dirs { | |||
| if err := os.MkdirAll(tmpDir, 0755); err != nil { | |||
| ui.PrintErr("Could not use temp dir ", tmpDir, ": ", err.Error()) | |||
| ui.Print("Could not use temp dir ", tmpDir, ": ", err.Error()) | |||
There was a problem hiding this comment.
I don't think it's the right way to fix the single test on a single platform, as it changes the behavior globally.
You can conditionally (for the problematic environment only) add a pattern to ignore in the configuration of the TestUI in the test,
pprof/internal/driver/fetch_test.go
Line 416 in 79c4198
I also wonder if the
proftest.go:114: Could not use temp dir /private/var/mobile/Containers/Data/Application/2B578AA2-4677-47C6-8665-DDB9509D5C92/pprof: mkdir /private/var/mobile/Containers/Data/Application/2B578AA2-4677-47C6-8665-DDB9509D5C92/pprof: operation not permitted
message from the test represents a real issue with temporary directory location on iOS. That can be handled separately though.
There was a problem hiding this comment.
setTmpDir tries $HOME/pprof first before os.TempDir. On iOS $HOME is the base directory of the installed app and isn't writable so I assumed the "Could not use temp dir" should be a warning, not an error.
However, I changed the PrintErr back and made your suggested change to the ignore pattern.
internal/driver/fetch_test.go
Outdated
| // CPU profiling is not supported on Plan9; see golang.org/issues/22564. | ||
| return | ||
| case "darwin": | ||
| if runtime.GOARCH == "arm" || runtime.GOARCH == "arm64" { | ||
| // CPU profiling is not supported on OS; see golang.org/issues/22612. |
There was a problem hiding this comment.
Is there a more useful bug to link, the one which actually tracks the profiling issue on iOS rather than points to the pprof's test breakage? Like we list golang.org/issues/22564 for Plan9.
There was a problem hiding this comment.
As per my comment above, I don't there is an issue with the basic CPU profiling machinery on iOS.
This will in turn fix golang.org/issues/22612 for the Go iOS builders.
eliasnaur
force-pushed
the
skip-test-on-ios
branch
from
6355876
to
40c2029
Compare
|
I've amended the PR to match your comments and suggestions. Sorry for the drive-by nature of this; I'm trying to get the Go iOS builder green again after refreshing the vendored copy of this package but I know very little about this package (nor the runtime/pprof package). |
|
The https://travis-ci.org/google/pprof/jobs/299036158 failure is unrelated, it's #255 |
This will in turn fix golang.org/issues/22612 for the Go iOS builders.
Also, replace a PrintErr with Print to avoid a warning message
being registered as an error.
This will in turn fix golang.org/issues/22612 for the Go iOS
builders.