1707282 - Replace testing "check" mechanism check-helper.sh script with rust "searchfox tool" and insta-based snapshot testing

Status: RESOLVED FIXED

(Webtools :: Searchfox, enhancement)

Description

[Andrew Sutherland [:asuth] (he/him)]

I'm filing this as a proposal that I'm currently planning to move ahead with, but am very much open to feedback about how wise it might be and whether any implementation synergies I'm thinking might exist actually exist!

Testing

Existing check mechanism: check-helper.sh

The current test mechanism is a minimal shell-script based mechanism that uses
check-helper.sh to potentially perform the following checks:

• Local check against the file-system:
  • Analysis file contains the given symbol somewhere.
  • Output file contains the given symbol somewhere.
• Server checks:
  • The served analysis file contains the given symbol somewhere.
  • The served HTML file contains the given symbol somewhere.
  • Querying for the given symbol results in a definition referencing the file
    in question.

The check directives in particular look like:
"$@" "simple.rs" "simple::Loader"

Proposal: rust-based CLI using subcommands wrapped by insta for snapshot testing

Clap is a command-line parser that supports sub-commands and can be used in conjunction with structopt to define the command line in terms of the data structure that will be populated. We would create a command line tool for searchfox that could be used as a command-line tool using these, with the binary being more of a nice side-effect than an actual goal. (Although I think there is some interest in such a CLI tool? See the (abandoned?) WIP https://github.com/brennie/searchfox-api) That said, the CLI I think could be pretty handy for investigating searchfox problems by automating a bunch of the investigation steps that currently involve a bunch of grep and jq and less.

For testing, this would be combined with snapshot / approval testing which I think I'd misleadingly described as expectation tests (which I believe implies expect-command style interleaving of command line expectations and their expected output in a single stream). insta seems like the best option for this given its level of documentation and tooling support like cargo-insta and a VSCode extension (although that's more for inline assert_json_snapshot uses).

The tentative plan would be to use the insta globbing feature so that we'd have a directory of input files that would look like command-lines you could invoke against the binary searchfox tool and then each input would have a corresponding snapshot output file. I could imagine 2 tiers of snapshotted outputs here:

• Curated "check"s: Manually selected symbols for analysis that are representative of interesting cases and in particular relate to patches as they land. This is our current test coverage.
• Regression: Maybe we snapshot every analysis file from the test repo (pretty-printed so the diffs are less nightmarish) as a general regression detection mechanism, but with the understanding that when non-trivial changes are made to the analysis process that people will only skim/randomly-sample the changes in this tier and it's expected that in such cases a curated check will be added.

Use of insta currently does imply that all of our tests will be somewhat shoe-horned into cargo test versus something like runt which is instead about running command lines for output. Given that insta seems to be most active and documented, this seems like a minor concern.

Example porting of command-line style

Edit Note: I initially thought we could easily get clap to support multiple sub-command invocations within a single command line in sequence, but I was mistaken. Explicitly: I thought query --symbol=foo and --symbol=bar would work as two sub-commands query and and. For my testing proposals, we're not actually involving the shell at all and can do whatever we want, so I'll see about instead handling the above via query --symbol=foo | and --symbol=bar, doing some kind of hacky parsing/splitting on the pipes. For the actual shell usage, I think it could still make sense to behave like jq where the invocation would still be quoted and passed through the super hacky fake pipeline layer, aka searchfox-tool 'query --symbol=foo | and --symbol=bar'.

• "$@" "simple.rs" "simple::Loader"
  • filter-analysis --file simple.rs --kind def --symbol simple::Loader
    • Find the definition(s) in the file, print them out.
    • Works on disk and network.
  • The above with | show-html tacked onto the end as a follow-on step.
    • Find and excerpt the produced HTML for the lines from the HTML output.
    • Works on disk and network.
  • query --symbol simple::Loader --kind def
    • Run a symbol query and filter the results to the definitions.
    • Currently would only work over the network, but if we teach the tool how to understand the crossref file (easier after https://bugzilla.mozilla.org/show_bug.cgi?id=1702916#c1) then this could also be done against disk.

One complication to this approach is that we potentially would have overlapping / redundant queries in some cases where we want to snapshot the output of both the first and 2nd steps there separately. Alternately each command stage could output into its own entry in a(n) (hetergogeneous) array of JSON output, but that seems confusing?

Command Line as (Advanced) Search Syntax?

Could the CLI style mechanism be a basis for the web query syntax?

All of our arguments that currently work like "symbol:FOO" could instead be "--symbol FOO" / "--symbol=FOO" / "-s FOO". And these could be thought of as occurring under the default "query"/"q" sub-command.

This idea seems appealing for situations like:

• Bug 1414954 on adding context support. Right now we have the secret syntax of context:N but arguably many searchfox users would already be familiar with the command line idiom of doing -C N and the extra knobs of -B/--before-context and -A/--after-context.

This provides a means of creating more advanced search pipelines. For example, imagine:

• An and sub-command that is just the default query command with intersection semantics and has options like -n/--near LINES with a default of 0 (for same-line). So the net command line (including assumed first sub-command of "query") could be something like query -C 3 FOO | and -n 5 BAR (and where I'm throwing in -C from above just to show composition... and I think it probably would want to be a global setting).
• A coverage sub-command that can filter based on methods that are covered/uncovered.
• A blame (or history) sub-command that can filter based on how recently (parts of) a method changed.

This could give examples like the following, noting that I'm using verbose argument syntax for readibility and assuming this is for a web syntax (which means the pipeline doesn't need the "searchfox" command everywhere), noting that we could potentially have the web UI expand short arguments to long arguments always for readability (or at least show a rendering of that if we don't modify the user's input).

• query --symbol=FOO --kind=use | or --symbol=BAR --kind=use | coverage --miss
  • Find all callers of FOO or BAR that don't have coverage.
• query --symbol=FooClass | methods | blame --since=fx80
  • Find all the methods of FooClass that have changed since Firefox 80.
  • This assumes a sub-command methods that is a graph traversal that takes us from a set of (presumably) classes to the unioned set of their methods (made possible via structured records).
• query --symbol=BarSuperclass | subclasses | methods | blame --since=fx80
  • Find all the methods of all of the subclasses which extend BarSuperclass that have changed since Firefox 80. An expansion on the above.
  • This assumes a sub-command subclasses that is a graph traversal from a set of (presumably) classes to the unioned set of their descendant classes. (Do we ever want just the immediate subclasses versus the transitive set of subclasses?)
• (query --symbol=nsIFoo subclasses) intersect (query --symbol=nsIBar subclasses)
  • This wouldn't actually work given what I've already proposed.
  • This captures one of the inherent tensions that any attempt at creating a query language faces where you are only ever creating a linear pipeline with no ability to create a tree. In order for the linear pipeline to work, the pipeline stages potentially have to be very complex. But it's also appealing to instead have simple traversal-like primitives.
    • In particular a major issue we face in searchfox is that the query mechanism as it currently exists is fuzzy, with us running provided query text against identifiers that are then expanded to symbols, plus fulltext search that we never map back to the underlying symbols/identifiers (although we could!).
    • Addressing this fuzziness might help, or it might just make searchfox a frustrating mess to use.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Initial searchfox-tool binary (landed)

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9222106 [details] [review]
Initial searchfox-tool binary (landed)

The initial searchfox-tool binary has landed, the next step is the "insta" test hookup and integration which will also involve populating the stubbed "query" command with a useless-to-humans query command, as it will just dump the resulting wall of JSON without any filtering.

We won't filter the results because the current results schema effectively has already performed all of the presentation decisions for the front-end and is not semantically useful.  I have a "sorch" variant schema that I would like us to move to which would be more suitable for processing by searchfox-tool, as it is basically the un-transformed results as they come from the crossref db.  (Noting that "sorch" should eventually replace "search", but we can phase it out gracefully, as I think there are at least some tools that do try and consume the current results and there's no need to break them abruptly.)

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Insta integration PR for mozsearch

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

This got delayed by the rustc nightly breakage, but now landed in:

• https://github.com/mozsearch/mozsearch/pull/425
• https://github.com/mozsearch/mozsearch-mozilla/pull/146

As noted at https://github.com/mozsearch/mozsearch-mozilla/pull/146#issuecomment-905105215 I validated this on the "asuth" channel but there's some level of potential for hiccups so I'll keep an eye on things. But in the event of serious regression, backing out the patches works or just moving the mozsearch-mozilla/mozilla-central/checks directory to not have that name will disable the checks.