Check the search-index generated by rustdoc tests

[mitaa]

This adds src/etc/search-index.py which can

• dump a {search, reference}-index as a reference-index
• check a {search, reference}-index for deviations
• update a reference-index using a {search, reference}-index

A reference-index is a simplified json-format of the search-index which is used for checking against the rustdoc generated search-index by embedding it into the tests source-file.

A diff between the reference- and search-index will be displayed should they deviate.

fixes #13444

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[brson]

Wow. This looks like it's doing something really cool. Are the comments in the rustdoc tests the 'reference index' mentioned in the code and does the script update the tests directly to replace the reference index? Do any of the new tests capture the corner cases @lifthrasiir mentioned in the issue?

Can you add some comments of search-index.py explaining what the script does, that it's run by compiletest on rustdoc tests, how to use it to update tests?

[mitaa]

Are the comments in the rustdoc tests the 'reference index' mentioned in the code

Yes

does the script update the tests directly to replace the reference index?

Well yes, but only one at a time through python search-index.py update source-file dest-file. Another way is to run python search-index.py dump index-file which outputs the fenced (/* !search-index ... */) reference-index ready for directly copying/embedding into the *.rs test-file.

Do any of the new tests capture the corner cases @lifthrasiir mentioned in the issue?

I'm not sure. I've found some obvious bugs but haven't looked too closely or specifically for what @lifthrasiir mentioned.

Can you add some comments of search-index.py explaining what the script does, that it's run by compiletest on rustdoc tests, how to use it to update tests?

Sure, I'll do that later when I have time.

There are two is one things I may want to change here:

• the reference-index does not contain the possibly present "parent-type", which is, I think, used for generating the link to the search-results rustdoc-page (i.e. an associated-type links to its parent items doc-page)
• I embedded the reference-index into the *.rs source-file because I didn't want to flood the src/test/rustdoc folder unnecessarily, but I've just noticed that you can't have a multi-line string in json, possibly a problem with long doc-comments which could clash with check-tidy (i.e. src/test/rustdoc/hidden-line.rs). So this may be better in a separate *.index file?
  edit: (though this issue could be avoided in rustdoc tests)

edit: nevermind, I forgot that python itself has various ways of dealing with such long lines...

[alexcrichton]

Nice work @mitaa! This seems like it'll definitely be useful for future refactorings of the search index. I idly wonder if it would be possible to write this script in Rust as we're generally trying to move away from Python where possible, but that can always come later.

Could you walk through what's necessary to add a new rustdoc test case now? I'm a little worried that it's going to get more difficult as now a python command needs to be run against the file by default. I also wonder if perhaps directives like the html checking may be more flexible instead of an exhaustive specification in all cases, but I'm not entirely sure what that would look like so it seems reasonable to start out this way.

[nikomatsakis]

I'm going to change the assignee here, since I don't think I'm the best choice.

r? @alexcrichton

[mitaa]

@alexcrichton

I'm a little worried that it's going to get more difficult as now a python command needs to be run against the file by default.

That is a good point.

Could you walk through what's necessary to add a new rustdoc test case now?

The only new requirement is to embed a "reference-index" in the test file, or to use the compiletest directive ignore-search-index.
Depending on the contents of the testcase it shouldn't be too hard to handwrite the expected reference-index, but generating it through the script may be easier/more reliable.
Using the script to dump/update the index you'd first need to generate the search-index either manually or using the make system.
If you want to insert the search-index rather than update a pre-existing one using the script you'd need to add /* !search-index */ to the test-file first so the script knows where to splice it in, though the update sub-command is more useful for adapting all rustdoc tests when changing the search-index output itself.
In concrete commands this may look like:

./configure ...
make check-stage1-rustdocck
# This will fail but builds the tests search-index
python src/etc/search-index.py dump x86_64-unknown-linux-gnu/test/rustdoc/testname.stage1-x86_64-unknown-linux-gnu/search-index.js

# Or compile manually, taking compiletest directives into account
rm -r doc
rustdoc src/test/rustdoc/testname.rs
python src/etc/search-index.py dump doc/search-index.js

And then copy the emitted reference-index straight into the test-file.

[alexcrichton]

Maybe the default should be to not check the search index? That way we could still add tests specifically for it, but adding tests by default wouldn't require the need to run a few extra commands to generate the output.

Also, could you make sure that the docs of the search-index.py script at the top prominently say what's supposed to be done to generate the reference index for a particular tests?

[mitaa]

I've flipped the compiletest directive skip-search-index to check-search-index and tried to improve the scripts documentation.

@alexcrichton
Does this look ok to you now?

[mitaa]

@alexcrichton

I idly wonder if it would be possible to write this script in Rust as we're generally trying to move away from Python where possible, but that can always come later.

I thought about this more and if you'd prefer I'd be okay trying to rewrite this in rust.

Related questions here:

• where would this 'script' go in the directory structure? A new ~'scripts' crate with multiple binaries?
• this probably would still require make integration? (not familiar with makefiles at all)
• how would/should someone go about invoking the script manually?
  A new build + install target for the script alone?

[alexcrichton]

@mitaa

Ah sorry for being a bit slow to get back to you on this, I mean to comment last week and then I forgot! To answer some of your questions:

• Thanks for the invert! I personally prefer the logic along these lines better this way, yeah.
• In terms of writing this in Rust, it's true that we don't have much prior support for this just yet. It's probably not quite worth it just yet anyway, just some musings of me.

I chatted with @brson about this briefly on IRC, and one interesting point was that this seems like this may end up having a high maintenance cost. Unfortunately you're probably the only one with a deep understanding of the verification script, so changes would likely have a high entry cost for others. Additionally, it's somewhat unclear if this will indeed catch regressions. I've noticed that the htmldockck script hasn't actually prevent a regression (to my knowledge), but that may just be because rustdoc doesn't see much active development so there's not a lot of churn.

The search index is also a curious thing here in terms of there's a pretty significant portion of JS which has to interpret and render the index. I suspect that it'd also be very difficult to catch regressions in that logic (and not possible via this script).

Overall we seemed a little unsure about whether we'd benefit from this in the long run (e.g. catch more regressions than the maintenance cost of keeping it up-to-date). I'm curious on your thoughts on this though? Do you think it'd be relatively easy to maintain over time?

[brson]

Even though I'm often in favor of more tests, I'm also uneasy about this for the reasons @alexcrichton states. Seems like it will be hard to maintain.

[mitaa]

This didn't quite come out as cleanly as I'd have liked, regarding implementation and actual usage. I can see that this might become a maintenance burden, so I'm fine closing this PR.

The main motivation here was the impending optimization of the search-index, but I'll try testing the index locally when the time comes.

Thanks!