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

Address Flakiness in ClearlyDefined API #2306

Merged

Conversation

robert-cronin
Copy link
Contributor

@robert-cronin robert-cronin commented Nov 20, 2024

Description of the PR

This PR attempts to address the recent flakiness observed in the ClearlyDefined API.
Fixes #2273
Fixes #2290

The main change is the addition of a Mock ClearlyDefined API that lets you provide coordinate -> definitions mapping directly.

Stress test results:

❯ stress go test -timeout 30s -race github.com/guacsec/guac/pkg/certifier/clearlydefined

5s: 55 runs so far, 0 failures, 8 active
10s: 112 runs so far, 0 failures, 8 active
15s: 171 runs so far, 0 failures, 8 active
20s: 228 runs so far, 0 failures, 8 active
25s: 286 runs so far, 0 failures, 8 active
30s: 344 runs so far, 0 failures, 8 active
35s: 400 runs so far, 0 failures, 8 active
40s: 459 runs so far, 0 failures, 8 active
45s: 515 runs so far, 0 failures, 8 active
50s: 573 runs so far, 0 failures, 8 active
55s: 633 runs so far, 0 failures, 8 active
1m0s: 690 runs so far, 0 failures, 8 active
1m5s: 747 runs so far, 0 failures, 8 active
1m10s: 805 runs so far, 0 failures, 8 active
1m15s: 862 runs so far, 0 failures, 8 active
1m20s: 920 runs so far, 0 failures, 8 active
1m25s: 976 runs so far, 0 failures, 8 active
1m30s: 1036 runs so far, 0 failures, 8 active
1m35s: 1093 runs so far, 0 failures, 8 active
1m40s: 1151 runs so far, 0 failures, 8 active
1m45s: 1208 runs so far, 0 failures, 8 active
1m50s: 1265 runs so far, 0 failures, 8 active
1m55s: 1324 runs so far, 0 failures, 8 active
2m0s: 1381 runs so far, 0 failures, 8 active

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If GraphQL schema is changed, GraphQL client updates/additions have been made
  • If OpenAPI spec is changed, make generate has been run
  • If ent schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@robert-cronin robert-cronin force-pushed the fix/address-flakiness-in-cd-api branch from 89a1b2e to 46145da Compare November 20, 2024 06:19
@robert-cronin
Copy link
Contributor Author

This does turn the test from a proper integration test into more of a unit test of the parsing pipeline, however I think it's okay since we still have the e2e testing to interface directly with api.clearlydefined.io. Open to any counter points or suggestions.

Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

This is great! This is also related to issue #2290, would you be able to change https://github.com/guacsec/guac/blob/main/pkg/ingestor/parser/common/scanner/scanner_test.go#L343 to use the mock? This can be done in a a separate PR.

@robert-cronin robert-cronin force-pushed the fix/address-flakiness-in-cd-api branch 2 times, most recently from 5dda6fd to 7c086a3 Compare November 21, 2024 05:10
Signed-off-by: robert-cronin <robert.owen.cronin@gmail.com>
@robert-cronin robert-cronin force-pushed the fix/address-flakiness-in-cd-api branch from 7c086a3 to 09a9534 Compare November 21, 2024 05:28
@kodiakhq kodiakhq bot merged commit af7cc26 into guacsec:main Nov 25, 2024
8 checks passed
@lumjjb
Copy link
Contributor

lumjjb commented Nov 25, 2024

❤️ ❤️ ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants