Closed mvdan closed 2 years ago
look, this is a gross hack.
Just use a test init function to set the variable, and it is much more preferable to do the ulimit thing. If it is platform dependent there are probably ways for each platform that we care, and there ought to be some library abstracting that.
On Mon, Sep 20, 2021, 18:28 Daniel Martí @.***> wrote:
@.**** commented on this pull request.
In pubsub_test.go https://github.com/libp2p/go-libp2p-pubsub/pull/450#discussion_r712278052 :
+func TestMain(m *testing.M) {
- // For all tests to work reliably in parallel,
- // we require being able to open many files at a time.
- // Many environments have lower limits,
- // such as Linux's "ulimit -n" soft limit defaulting to 1024.
- // On a machine with 16 CPU threads, 8k seems to be enough.
- const wantFileLimit = 8 << 10
- files := make([]*os.File, 0, wantFileLimit)
- for i := 0; i < wantFileLimit; i++ {
- file, err := os.Open("pubsub_test.go")
- if err != nil {
- if i == 0 {
- panic(err) // the file doesn't exist?
- }
- fmt.Fprintf(os.Stderr, "Skipping parallel runs of tests; open file limit %d is below %d.\n",
- i, wantFileLimit)
- fmt.Fprintf(os.Stderr, "On Linux, consider running: ulimit -Sn %d\n",
- wantFileLimit*2)
- canRunInParallel = false
- break
- }
- files = append(files, file)
- }
- for _, file := range files {
- file.Close()
- }
- os.Exit(m.Run()) +}
I'm not sure I understand. We could do this kind of check for every test, but that feels very repetitive, and also far too specific. Are we going to measure exactly how many FDs each tests consumes?
The limits also don't apply to each test in isolation. It's roughly "number of FDs used at a time per test on average" multiplied by GOMAXPROCS. So I think a global approximation is good enough overall. Right now CI uses 2k, and I locally found 8k to be plenty with 16 CPU threads, so that seems fine to me, at least.
I also thought about doing the equivalent of ulimit -Sn directly, but note that that's not portable, so it would require code behind // +build linux that would not work on other systems. It also seems a bit weird for tests to lift the soft limit on their own; it feels like something the user should be in control of, like what the CI config does.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libp2p/go-libp2p-pubsub/pull/450#discussion_r712278052, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI4SUVGQXFIOONH2J5TB3UC5HILANCNFSM5EMFRWPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
Just use a test init function to set the variable
Sorry, but I'm still not understanding that approach. I think a global one is reasonable, as I tried to explain in https://github.com/libp2p/go-libp2p-pubsub/pull/450#discussion_r712278052.
there ought to be some library abstracting that.
I would agree, but it doesn't seem to exist. I'm more than happy to use one if you point me towards it. Otherwise, just making it work on Linux doesn't feel particularly better than what this PR has right now.
look, this is a gross hack.
All that said, I'll close the PR if you don't feel like this is worth your time. I honestly thought making the tests not take four minutes would be worthwhile for everyone. Any solution we come up with will be relatively hacky in one way or another, because of varying environments and defaults.
look, i am not merging this code, the open files hack is horrible.
If you are looking for an easy way to parallelize the tests, then use an env variable and trst it from an init function in the test.
On Tue, Sep 21, 2021, 16:39 Daniel Martí @.***> wrote:
Just use a test init function to set the variable
Sorry, but I'm still not understanding that approach. I think a global one is reasonable, as I tried to explain in #450 (comment) https://github.com/libp2p/go-libp2p-pubsub/pull/450#discussion_r712278052 .
there ought to be some library abstracting that.
I would agree, but it doesn't seem to exist. I'm more than happy to use one if you point me towards it. Otherwise, just making it work on Linux doesn't feel particularly better than what this PR has right now.
look, this is a gross hack.
All that said, I'll close the PR if you don't feel like this is worth your time. I honestly thought making the tests not take four minutes would be worthwhile for everyone. Any solution we come up with will be relatively hacky in one way or another, because of varying environments and defaults.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libp2p/go-libp2p-pubsub/pull/450#issuecomment-923999355, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI4STZDGGDMEPEJPAVAADUDCDHZANCNFSM5EMFRWPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
(see commit message)