Skip to content

Turn inline expectation test into a parameterized module #12789

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

Merged
merged 5 commits into from
May 23, 2023

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Apr 8, 2023

No description provided.

predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
predicate hasOptionalResult(Impl::Location location, string element, string tag, string value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could have a default none() implementation.

Copy link
Contributor Author

@jketema jketema Apr 11, 2023

Choose a reason for hiding this comment

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

Can we do that now? Note that this predicate refers to part of the signature: Impl::Location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see we have an internal PR open that'll likely help here. We should probably hold this off until that has landed in a release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, you are right, we can't do that just yet.

Copy link
Contributor Author

@jketema jketema May 23, 2023

Choose a reason for hiding this comment

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

@hvitved none() implementation added. I assume we still need to wait with this PR until support for referencing other parts of the module signature in defaults is available in a release (which it isn't yet, should be 2.13.4)?

Copy link
Contributor Author

@jketema jketema May 23, 2023

Choose a reason for hiding this comment

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

I just doubled checked and it seems that the default here was not working for some other reason when I last tried this. Testing this with CodeQL 2.13.1 the default for this seems to work out-of-the-box.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Compile all queries using the latest stable CodeQL CLI check should be enough to ensure that it works with the latest release.

@jketema jketema marked this pull request as ready for review May 23, 2023 11:43
@jketema jketema requested a review from a team as a code owner May 23, 2023 11:43
@@ -0,0 +1,2 @@
failures
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit of a shame that we also have the failures predicate here, but I don't see an easy way to avoid that. Should of course disappear when we have converted all existing tests away from the legacy interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

2 participants