Skip to content

Java: Tests for Automodel Extraction Queries #13788

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Aug 1, 2023
Merged

Java: Tests for Automodel Extraction Queries #13788

merged 17 commits into from
Aug 1, 2023

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented Jul 21, 2023 •
edited
Loading

This PR adds some tests for the automodel extraction queries. @adityasharad will review this, but other reviewers are always welcome.

  • The tests come with hand-written Java code, so that they can also serve as documentation of intent (see the comments in the java files).
  • This does only add tests, not change the queries themselves (edit: an exception: fixed a badly named query).
  • Taus suggested, in the (backref'd) issue to add some models just for testing. I've found this to not be necessary, as we can easily test modeled classes by 're-implementing' them. Eg., there's a class java.io.File implemented in the test suite. I find this much easier than the suggested implementation. I was hoping this would work, and I'm glad it does.

How to review/Questions for the reviewers:

  • We'll add more test cases to these suites as we maintain the queries, but are there important features you'd like to see tested straight away?
  • Do you have out of the box ideas to improve this?
  • The intention is to set up a small test-suite that'll make it easier to add tests of new and existing behaviour in the future.
    The reason why this only tests some of our queries (only the extraction queries; the queries share most of the implementations behind the scenes) is to manage the maintenance overhead of the test suites. LMK if you disagree with that and would like to see more test suites.

@github-actions github-actions bot added the Java label Jul 21, 2023
@kaeluka kaeluka changed the title Tests for Automodel Extraction Queries Java: Tests for Automodel Extraction Queries Jul 21, 2023
@kaeluka kaeluka force-pushed the kaeluka/automodel-telemetry-testing branch from c78c031 to bee43b0 Compare July 24, 2023 08:34
@kaeluka kaeluka marked this pull request as ready for review July 24, 2023 10:08
@kaeluka kaeluka requested a review from a team as a code owner July 24, 2023 10:08
@kaeluka
Copy link
Contributor Author

kaeluka commented Jul 24, 2023

After merging #13747, this needed minor updates 👍

@kaeluka kaeluka added the no-change-note-required This PR does not need a change note label Jul 24, 2023
adityasharad
adityasharad reviewed Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Glad to see tests! Some high level comments on structure before I get into checking the test cases themselves.

@kaeluka
Copy link
Contributor Author

kaeluka commented Jul 25, 2023

following @adityasharad's comments, this has been restructured significantly. Before merging this, I'd like to squash these commits a bit.

@kaeluka kaeluka force-pushed the kaeluka/automodel-telemetry-testing branch 2 times, most recently from 64d38ef to 9c51f4d Compare July 26, 2023 11:40
@kaeluka kaeluka mentioned this pull request Jul 26, 2023
Merged
adityasharad
adityasharad reviewed Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Good stuff. A few minor comments.

kaeluka added 16 commits August 1, 2023 09:18
@kaeluka
Java: tests for automodel framework mode candidate extraction
18fe587
@kaeluka
Java: tests for automodel application mode candidate extraction
2e89a11
@kaeluka
Java: tests for automodel application mode positive example extraction
1bc222e
@kaeluka
Java: tests for automodel framework mode positive example extraction
abed936
@kaeluka
Java: tests for automodel application mode negative example extraction
938a7a7
@kaeluka
Java: tests for automodel framework mode negative example extraction
457604e
@kaeluka
Java: tests for automodel application mode, test that local calls are…
5060310 
… not candidates
@kaeluka
Java: update extraction query tests after merging PR #13747
37b6b46
@kaeluka
Java: merge application mode tests into one
8cc367c
@kaeluka
Java: merge framework mode tests into one
44b8ec6
@kaeluka
Java: Automodel tests: update after merging #13818
f5c4155
@kaeluka
Java: Automodel package private test case
be629b2
@kaeluka
Java: fix a comment
da87d82
@kaeluka
Java: update text expectations after merging #13823
5ad984f
@kaeluka
Java: Drive-by: fix oversight in #13823
0582368 
In PR #13823, we had rewritten the endpoints that are being considered for framework mode. We used to use `DataFlow::ParameterNode` as endpoints.
However, `ParameterNode`s do not exist for the implicit `this` parameter; they also do not exist for bodiless interface-methods.

In PR #13823, we forgot to model that `this` only exists for non-static methods and to only consider parameters that we have source code for.
@kaeluka
Java: add automodel framework mode test case for newly supported inte…
bc3e78f 
…rface-method parameter extraction
@kaeluka kaeluka force-pushed the kaeluka/automodel-telemetry-testing branch from b73d869 to 621c05d Compare August 1, 2023 07:19
@kaeluka
Copy link
Contributor Author

kaeluka commented Aug 1, 2023

@adityasharad, do you think this can be merged? If so, do you think I should squash these commits a bit, or would you merge as-is?

adityasharad
adityasharad approved these changes Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@adityasharad adityasharad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. I also verified that the new tests did actually run in the Java Language Tests run. Commits are clear enough I think, no need to squash.

@kaeluka kaeluka merged commit cb55b10 into main Aug 1, 2023
@kaeluka kaeluka deleted the kaeluka/automodel-telemetry-testing branch August 1, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atorralba atorralba atorralba left review comments

@jcogs33 jcogs33 jcogs33 left review comments

@adityasharad adityasharad adityasharad approved these changes

Assignees

@adityasharad adityasharad

Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kaeluka @adityasharad @atorralba @jcogs33