Skip to content

JS: use query predicate instead of select for testing Express#999

Closed
ghost wants to merge 1 commit into
masterfrom
unknown repository
Closed

JS: use query predicate instead of select for testing Express#999
ghost wants to merge 1 commit into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Feb 27, 2019

Copy link
Copy Markdown

This speeds up the tests for Express since the tests now are compiled
together: the total test time for the affected directory was 1102 seconds
before this change, and 48.7 seconds after: a 95% time reduction.

Besides the speedup, a major difference between the two test styles is
that all selected entities now must have an explicit type, this is a
bit more verbose, but also produce slightly better tests.

I have picked a naming convention for the query predicates: they all
start with "test_" to allow for writing capitalized class names.

I expect similar improvements for other large test directories to
trickle in later.

Process for doing this rewrite: cat *.ql >> Test.ql, and then
rewrite manually from the top. This is slightly suboptimal: I can not
be entirely sure the rewritten tests are equivalent. :(

A better process would have been to rewrite each test in isolation,
and then concatenate them into one big file afterwards.

@ghost ghost added the JS label Feb 27, 2019
@ghost ghost self-requested a review as a code owner February 27, 2019 15:00
asger-semmle
asger-semmle previously approved these changes Feb 27, 2019

@asger-semmle asger-semmle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is seriously awesome!

I obviously haven't checked that the rewrite was correct, but the naming convention and resulting .ql LGTM.

@ghost

ghost commented Feb 27, 2019

Copy link
Copy Markdown
Author

https://jira.semmle.com/browse/ODASA-7810.
In case you want more:

find . -type f -name \*.ql | sed -e 's+\(.*\)/.*+\1+' | sort | uniq -c | sort -rn | head -n 20
29 ./library-tests/TypeScript/TypeAnnotations
24 ./library-tests/frameworks/NodeJSLib
20 ./tutorials/Introducing the JavaScript libraries
20 ./library-tests/JSDoc
18 ./library-tests/Modules
18 ./library-tests/frameworks/koa
18 ./library-tests/Classes
16 ./library-tests/Functions
15 ./library-tests/frameworks/connect
14 ./library-tests/frameworks/restify
14 ./library-tests/frameworks/hapi
12 ./library-tests/PropWrite
11 ./library-tests/ModuleImportNodes
11 ./library-tests/Expr
11 ./library-tests/CallGraphs
10 ./library-tests/stmts
10 ./library-tests/Promises
10 ./library-tests/frameworks/ReactJS
10 ./library-tests/DOM
9 ./library-tests/frameworks/UriLibraries

@asger-semmle

Copy link
Copy Markdown
Contributor

Could you rebase now that the failing test on master has been fixed?

This speeds up the tests for express since the tests now are compiled
together: the total test time for the affected directory was 1102 seconds
before this change, and 48.7 seconds after: a 95% time reduction.

Besides the speedup, a major difference between the two test styles is
that all selected entities now must have an explicit type, this is a
bit more verbose, but also produce a slightly better tests.

I have picked a naming convention for the query predicates: they all
start with "test_" to allow for writing capitalized class names.

I expect similar improvements for other large test directories to
trickle in later.

-

Process for doing this rewrite: `cat *.ql >> Test.ql`, and then
rewrite manually from the top. This is slightly suboptimal: I can not
be sure the rewritten tests are equivalent. :(

A better process would have been to rewrite each test in isolation,
and then concatenate them into one big file afterwards.
@xiemaisi

Copy link
Copy Markdown

(Please don't merge just yet, I'm trying out an alternative approach I'd like to show for comparison.)

@ghost ghost added the WIP This is a work-in-progress, do not merge yet! label Feb 28, 2019
@xiemaisi

xiemaisi commented Mar 5, 2019

Copy link
Copy Markdown

Superseded by #1018.

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

Labels

JS WIP This is a work-in-progress, do not merge yet!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants