-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Automodel: Switch tests to inline expectations #15356
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
Automodel: Switch tests to inline expectations #15356
Conversation
The apparently missing test result is due to sampling.
…ts results before sampling.
...automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractionTests.ql
Fixed
Show fixed
Hide fixed
.../ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractionTests.ql
Fixed
Show fixed
Hide fixed
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.
Thanks for a very nice PR.
I've left some inline comments, but no blocker.
This entails factoring the where clauses of our queries into predicates we can use in the tests, but I think that's a good idea anyway since it allows us to get the full results of the queries without sampling if desired,
💯 yes, as we have noticed, the sampling used in some extraction queries makes the testing very tedious and also error-prone (eg., when a candidate goes away it's not obvious whether the query logic removed it, or the sampling did).
FYI, @tausbn: you will be pleased to hear we'll finally be using inline expectation tests (I think you created an issue sometime to do exactly that, but I can't find it 😬)
java/ql/automodel/test/AutomodelApplicationModeExtraction/PluginImpl.java
Outdated
Show resolved
Hide resolved
java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java
Outdated
Show resolved
Hide resolved
I think maybe I never wrote up an issue. I just groused about it a lot on various PRs. 🙂 Anyway, yay! 🎉 |
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.
Looks great!
I also restarted a failing test (looked flaky)
This entails factoring the
where
clauses of our queries into predicates we can use in the tests, but I think that's a good idea anyway since it allows us to get the full results of the queries without sampling if desired, which can be quite useful when comparing results from multiple runs (since the sampling makes diffs very noisy).I also cleaned up the treatment of unexploitable types (primitives, boxed types, and
void
). Previously, some candidates with such types were not suppressed in framework mode, and in both modes we had two overlapping characteristics for excluding them, leading to duplicate results for the negative-examples queries.Consequently, on
apache/geode
the number or negative examples has gone down a bit (519 -> 430 in application mode, 6215 -> 6146 in framework mode), and so has the number of candidates in framework mode (40326 -> 39065).