-
Notifications
You must be signed in to change notification settings - Fork 12
Add load tests for expansion analyzers #755
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: jackfirth <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resyntax analyzed 1 file in this pull request and found no issues.
jackfirth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put all of this inside of a (module+ test ...) submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 1ab6849. All code is now inside (module+ test ...).
test/analyzer-load-test.rkt
Outdated
| (define analysis | ||
| (source-analyze large-test-source | ||
| #:analyzers (list variable-mutability-analyzer) | ||
| #:timeout-ms timeout-ms)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. The source-analyze will kill the analyzer after timeout-ms and return an empty source-analysis? value while logging a warning. So the test cases in this file will always pass.
Instead of using source-analyze, call the expansion analyzer directly (with expansion-analyze) by passing it fully-expanded syntax created using source-expand in a new base namespace. Use time-apply to measure how long the analyzer takes to process the expanded syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1ab6849. Now using expansion-analyze directly with time-apply to measure actual analyzer performance. The syntax is expanded once using source-expand and reused for all tests.
test/analyzer-load-test.rkt
Outdated
| (test-case "all analyzers together load test" | ||
| ;; This test ensures all analyzers can work together on a large file | ||
| ;; within the timeout period. | ||
| (define all-analyzers | ||
| (list identifier-usage-analyzer | ||
| variable-mutability-analyzer | ||
| ignored-result-values-analyzer | ||
| function-expression-analyzer)) | ||
|
|
||
| (define start-time (current-inexact-milliseconds)) | ||
| (define analysis | ||
| (source-analyze large-test-source | ||
| #:analyzers all-analyzers | ||
| #:timeout-ms timeout-ms)) | ||
| (define elapsed-ms (- (current-inexact-milliseconds) start-time)) | ||
|
|
||
| ;; Verify the analysis completed successfully | ||
| (check-true (source-code-analysis? analysis)) | ||
|
|
||
| ;; Verify it completed within the timeout (with some margin) | ||
| (check-true (< elapsed-ms timeout-ms) | ||
| (format "all analyzers together took ~a ms, expected < ~a ms" | ||
| elapsed-ms | ||
| timeout-ms))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't bother with this load test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 1ab6849.
- Wrap everything in (module+ test ...) submodule - Use expansion-analyze directly instead of source-analyze - Use time-apply to measure timing - Expand syntax once and reuse for all tests - Remove "all analyzers together" test Co-authored-by: jackfirth <[email protected]>
Expansion analyzers can take excessive time on large files. This adds load tests to catch performance regressions.
Changes
test/analyzer-load-test.rkt: Load tests for all four expansion analyzers (identifier-usage,variable-mutability,ignored-result-values,function-expression)expansion-analyzedirectly withtime-applyto measure actual analyzer performancesource-expandand reuses it for all tests(module+ test ...)submoduleEach test measures elapsed time and fails with diagnostic message if timeout exceeded:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.