C#: Convert qlref tests to inline expectations#22123
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owen-mc
force-pushed
the
csharp/convert-qlref-tests-to-inline-expectations
branch
2 times, most recently
from
800482d to
c0273ba
Compare
owen-mc
force-pushed
the
csharp/convert-qlref-tests-to-inline-expectations
branch
from
8579a93 to
69c4d4e
Compare
owen-mc
force-pushed
the
csharp/convert-qlref-tests-to-inline-expectations
branch
from
31fb2e6 to
f67b729
Compare
|
I've never had CodeQL alerts from editing test files in any other language I've interacted with. Is there perhaps some configuration change that could be made so that CodeQL doesn't scan the test folders? |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
What's going on with the last commit ("Revert inline expectations for blazor integration tests with BMN") - should these be marked as MISSING: results???
| C1 Field2; // BAD | ||
|
|
||
| public virtual void Dispose() | ||
| public virtual void Dispose() // $ Alert |
There was a problem hiding this comment.
The alert locations in this file aren't super helpful, it's really the field (which is in the alert message) that's of interest. I'm not sure what to suggest. The .expected file should still contain the full alert message anyway.
There was a problem hiding this comment.
For this PR I think it's fine as it is. In an ideal world we could refactor the logic from the ql file into a .qll file (or use a postprocessing query) and make a more in-depth inline expectations test. But I think our time is better spent elsewhere.
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
|
The last commit ("Revert inline expectations for blazor integration tests with BMN") is there because it turns out that in BMN mode, we don't extract razor comments. This is probably fixable, and I've given the suggestions from copilot about how to approach it to the relevant team, but I believe it is out of the scope of this PR. (And it may not be worth it at all, if this is the only use case.) To make this PR mergeable I have taken the decision to not use inline expectations for the two integration tests that this affects. |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing / answering my comments.
Convert suitable qlref tests to inline expectations. Done by Copilot (Opus 4.8).