Skip to content
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

Integration test for node false negative pattern #895

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

esohel30
Copy link
Contributor

adding integration test for node false negative pattern.

Copy link

@tianhan0 tianhan0 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

Could you also include the expected results for this test (e.g., the *.models files)? Otherwise the CI tests would fail.
You can run Pysa on all integration tests by executing OUNIT_SHARDS=1 dune exec interprocedural_analyses/taint/test/integrationTest.exe under pyre/source. This would

  1. either create new expected results for new tests (such as this PR), because there is no existing expected results.
  2. or create the actual results, when they differ from the existing expected results.

Additionally, could we

  1. rename this test to be more descriptive, such as heap_alias.py? We try to let the name of each integration test be as descriptive as possible.
  2. add a few comments in the .py file to explain why this is a false negative. You could paraphrase Maxime's explanation (in the chat).

@kinto0
Copy link
Contributor

kinto0 commented Aug 14, 2024

cc @tianhan0 for updated review

@kinto0 kinto0 requested a review from tianhan0 August 14, 2024 18:11
@tianhan0
Copy link

cc @tianhan0 for updated review

@kinto0 I don't think this PR has been updated in the past month. But we will keep an eye on this -- if the author (who is an MLH fellow) does not update this in another month or so, we will abandon this PR and recreate it internally.

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

Successfully merging this pull request may close these issues.

4 participants