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

tests: don't mutate global structs in core scheduler tests #16120

Merged
merged 3 commits into from
Feb 10, 2023

Conversation

tgross
Copy link
Member

@tgross tgross commented Feb 9, 2023

Some of the core scheduler tests need the maximum batch size for writes to be smaller than the usual structs.MaxUUIDsPerWriteRequest. But they do so by unsafely modifying the global struct, which creates test flakes in other tests.

Modify the functions under test to take a batch size parameter. Production code will pass the global while the tests can inject smaller values. Turn the structs.MaxUUIDsPerWriteRequest into a constant, and add a semgrep rule for avoiding this kind of thing in the future.

Discovered while trying to debug #16112, but this should get backported to all supported versions whereas that will be only to 1.4.x

Some of the core scheduler tests need the maximum batch size for writes to be
smaller than the usual `structs.MaxUUIDsPerWriteRequest`. But they do so by
unsafely modifying the global struct, which creates test flakes in other
tests.

Modify the functions under test to take a batch size parameter. Production code
will pass the global while the tests can inject smaller values.
@tgross tgross added the theme/testing Test related issues label Feb 9, 2023
@tgross tgross added this to the 1.5.0 milestone Feb 9, 2023
@tgross tgross added backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line labels Feb 9, 2023
@tgross tgross marked this pull request as ready for review February 9, 2023 21:54
@tgross
Copy link
Member Author

tgross commented Feb 9, 2023

New semgrep rule from this PR, without the patch to the tests:

$ semgrep scan -c ./.semgrep/protect_globals.yml
METRICS: Using configs from the Registry (like --config=p/ci) reports pseudonymous rule metrics to semgrep.dev.
To disable Registry rule metrics, use "--metrics=off".
Using configs only from local files (like --config=xyz.yml) does not enable metrics.

More information: https://semgrep.dev/docs/metrics

Scanning 1683 files.

Findings:

  nomad/core_sched_test.go
     semgrep.no-overriding-struct-globals
        Mutating global structs is never safe

       1853┆ structs.MaxUUIDsPerWriteRequest = 2
          ⋮┆----------------------------------------
       1896┆ structs.MaxUUIDsPerWriteRequest = 2
          ⋮┆----------------------------------------
       1930┆ structs.MaxUUIDsPerWriteRequest = 2
          ⋮┆----------------------------------------
       1932┆ structs.MaxUUIDsPerWriteRequest = originalMaxUUIDsPerWriteRequest

Some files were skipped or only partially analyzed.
  Scan was limited to files tracked by git.
  Partially scanned: 3 files only partially analyzed due to a parsing or internal Semgrep error
  Scan skipped: 1 files larger than 1.0 MB, 1 files matching .semgrepignore patterns
  For a full list of skipped files, run semgrep with the --verbose flag.

Ran 1 rule on 1683 files: 4 findings.

nomad/structs/uuid.go Outdated Show resolved Hide resolved
@tgross tgross merged commit ce614bf into main Feb 10, 2023
@tgross tgross deleted the b-flaky-core-sched-tests branch February 10, 2023 14:26
tgross added a commit that referenced this pull request Feb 10, 2023
Some of the core scheduler tests need the maximum batch size for writes to be
smaller than the usual `structs.MaxUUIDsPerWriteRequest`. But they do so by
unsafely modifying the global struct, which creates test flakes in other tests.

Modify the functions under test to take a batch size parameter. Production code
will pass the global while the tests can inject smaller values. Turn the
`structs.MaxUUIDsPerWriteRequest` into a constant, and add a semgrep rule for
avoiding this kind of thing in the future.
tgross added a commit that referenced this pull request Feb 10, 2023
…16123)

Some of the core scheduler tests need the maximum batch size for writes to be
smaller than the usual `structs.MaxUUIDsPerWriteRequest`. But they do so by
unsafely modifying the global struct, which creates test flakes in other tests.

Modify the functions under test to take a batch size parameter. Production code
will pass the global while the tests can inject smaller values. Turn the
`structs.MaxUUIDsPerWriteRequest` into a constant, and add a semgrep rule for
avoiding this kind of thing in the future.

Co-authored-by: Tim Gross <tgross@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line theme/testing Test related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants