Skip to content

JS: show test changes after #15823 #15883

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 3 commits into from
Mar 19, 2024
Merged

JS: show test changes after #15823 #15883

merged 3 commits into from
Mar 19, 2024

Conversation

RasmusWL
Copy link
Member

This PR is a journey for me into better understanding the JS CodeQL analysis, by trying to write tests to show the effect of #15823. I've written some tests that initially fail on commit 245cd5c (parent of PR merge commit), and then tested them again on 7c35309 (PR merge commit)

I was able to highlight the effect of the Detect more FunctionStyleClasses commit, see the change in javascript/ql/test/ApiGraphs/classes/VerifyAssertions.expected.

I had expected that the step defined here

CallGraph::impliedReceiverStep(pred, succ) and
summary = CallStep()
would cause at least the obj_ type-tracking test to pass after the changes in the PR, but that was not the case.

So I wasn't able to write a test that highlights the impact of the change in f5d014b, even though I can see the change in impliedReceiverStep when quick-eval'ing locally. @asgerf can you highlight the end-result impact of that commit? 😕

@github-actions github-actions bot added the JS label Mar 12, 2024
@@ -0,0 +1,24 @@
/** name:foo */
Copy link
Contributor

Choose a reason for hiding this comment

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

Try putting import 'dummy'; at the top so the file is seen as a module. Otherwise all variables at the top-level become global variables which are tracked less precisely.

Copy link
Member Author

@RasmusWL RasmusWL Mar 15, 2024

Choose a reason for hiding this comment

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

That changed initial test results (in positive way), but I still don't see impact of implicit receiver step in the test changes after merging in the PR: ac05c80

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously there was a requirement that the host object was an object literal or function. That restriction was lifted in the PR, but in these test, it seems the host object is always an object literal, so they are not affected by the change.

@RasmusWL
Copy link
Member Author

After some more back and forth locally, we finally managed to make a nice test example of the implicit receiver step 👍

@RasmusWL RasmusWL marked this pull request as ready for review March 18, 2024 12:11
@RasmusWL RasmusWL requested a review from a team as a code owner March 18, 2024 12:11
@RasmusWL RasmusWL merged commit d78efdb into github:main Mar 19, 2024
@RasmusWL RasmusWL deleted the js-cg-tests branch March 19, 2024 08:58
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