Improve rustdoc JS tests error output #79443

GuillaumeGomez · 2020-11-26T13:05:53Z

It's pretty common when starting to add new tests for rustdoc-js to have issues to understand the errors. With this, it should make things a bit simpler. So now, in case of an error, it displays:

---- [js-doc-test] rustdoc-js/basic.rs stdout ----

error: rustdoc-js test failed!
failed to decode compiler output as json: line: {
output: Checking "basic" ... FAILED
==> Result not found in 'others': '{"path":"basic","name":"Fo"}'
Diff of first error:
{
    "path": "basic",
    - "name": "Fo",
    + "name": "Foo",
}
thread '[js-doc-test] rustdoc-js/basic.rs' panicked at 'explicit panic', src/tools/compiletest/src/json.rs:126:21

I think it was @camelid who asked about it a few days ago?

r? @jyn514

camelid · 2020-11-26T19:58:11Z

src/tools/rustdoc-js/tester.js

+                output += '    + ' + contentToDiffLine(key, value) + '\n';
+                error = true;
+            } else {
+                output += '    ' + contentToDiffLine(key, value) + '\n';


This looks much better, thanks! Having a diff is really helpful :)

One thing I noticed though is that the context lines don't get the same indentation as the changed lines. Generally diffs have the same ultimate indentation for every line so that it's easier to read. Anyway, this should fix it:

Suggested change

output += ' ' + contentToDiffLine(key, value) + '\n';

output += ' ' + contentToDiffLine(key, value) + '\n';

It may make sense to instead remove two spaces from the changed lines so that there isn't too much indentation. But not really a big deal either way :)

I'll remove the two whitespaces instead. ;)

So this is the result:

Diff of first error: { "path": "basic", - "name": "Fo", + "name": "Foo", }

Sorry but I'll revert that change. It actually makes spotting the diff harder. :-/

I think it would be better if you made it look like diff output:

{ "path": "basic", - "name": "Fo", + "name": "Foo", }

Let me check how it looks like in the terminal (without colors, it might not be that great...).

With the new output:

Diff of first error: { "path": "basic", - "name": "Fo", + "name": "Foo", }

If it's good for both of you, I push this?

Why do the changed lines have one space extra of indentation though? Usually changed lines have the same indentation of context lines.

I actually preferred the style you used before, with the - and + closer to the field ;). But we can always change this later anyway.

jyn514

LGTM modulo Camelid's nit and my suggestion.

src/tools/rustdoc-js/tester.js

GuillaumeGomez · 2020-11-26T20:37:31Z

Updated!

jyn514 · 2020-11-26T20:43:49Z

src/tools/rustdoc-js/tester.js

+function betterLookingDiff(entry, data) {
+    let output = '{\n';
+    data = data[0];


Maybe it's better to pass in the first element, instead of passing in the whole array but only using the first one?

I made something completely different first where we had a more "global" diff. The result wasn't great so I dropped it but maybe we will give it another try in the future. So in this case, I think it's better to keep it this way. What do you think?

Hmm, can you say more about why it wasn't great? I still think it makes sense to show all the differences instead of just the first.

Because generally you have one item in the tests whereas the search will return you up to a few hundreds. It wouldn't make sense to make the diff between all of them. However, you made me realize something so I'll improve it once more! :D

Hum no, not gonna work either haha.

Can you print the diff only for items that changed, instead of printing all of them unconditionally?

You call this function in case you didn't find a matching item. How do I know which is the correct one then? :)

That's also why I made the small improvement in case order matters, so that you get the first item that you should have gotten.

GuillaumeGomez · 2020-11-26T21:00:30Z

Ok, so I improved the choice of which item was compared and updated the diff output.

camelid · 2020-11-26T21:14:54Z

Can you show what the output looks like now? I don't think the diff output will be aligned correctly.

Also, does this have the possibility of choosing the wrong search result to diff against and thus show weird and confusing diffs?

GuillaumeGomez · 2020-11-27T12:16:48Z

I showed it here, but here it is again 😉 :

Diff of first error:
 {
     "path": "basic",
-     "name": "Fo",
+     "name": "Foo",
 }

jyn514 · 2020-11-27T18:46:18Z

src/tools/rustdoc-js/tester.js

+                // By default, we just compare the two first items.
+                let item_to_diff = 0;
+                if ((ignore_order === false || exact_check === true) && i < results[key].length) {
+                    item_to_diff = i;
+                }


This also seems pretty odd to me - it means that if ignore_order isn't set, this might not even compare the right set of objects!

I think a good way to resolve all the concerns both I and @camelid have mentioned is to unconditionally print all the objects, then show a diff between that and all the expected objects. That means:

The order won't matter (assuming you print them sorted)

All objects that don't match will be shown

It will use diff syntax without having to worry too much about exactly how to print +- . (I'm imagining that you just call the diff CLI tool, but you could also use something like https://docs.rs/patch/0.5.0/patch/).

If an object is missing altogether, that will show up in the diff, and you'll be able to tell the difference between that and rustdoc guessing the wrong object to compare to.

The problem with the diff CLI tool is that you need at least one of the arguments to be a file. The problem with the patch crate is this is JavaScript :P

Ok, it could create a temporary file and then delete it after then.

And like I said: search can return hundreds of results. You don't want to compare them all. We show the failing comparison, I think it provides more than enough information.

Also, I don't want to rely on external tools that might not be installed (try your luck on windows, you'll see how "easy" it is to setup pathes to binaries hehe).

We show the failing comparison, I think it provides more than enough information.

But we don't show the failing comparison if ignore_order isn't set - we show the incorrect object and a random other object that happened to come first. They might have nothing to do with one another!

You don't want to compare them all.

Right, diff will only show the ones that changed. I don't think we should be trying to second guess which changes are important and which aren't when we don't have the information to tell.

We show the failing comparison, I think it provides more than enough information.

But we don't show the failing comparison if ignore_order isn't set - we show the incorrect object and a random other object that happened to come first. They might have nothing to do with one another!

Exactly, but we have literally no way to guess which one was supposed to be the matching one in this case. And you still don't want hundreds of comparisons (all failing).

You don't want to compare them all.

Right, diff will only show the ones that changed. I don't think we should be trying to second guess which changes are important and which aren't when we don't have the information to tell.

I can make a special case for the remaining one and just show the object saying "this object was not found in the data", but then you might miss an information. Generally, when you fail this test, it's simply because you badly set one of the fields (the parent one generally), which you will spot right away with this diff. I think it's really an improvement because of that.

Well, I guess in the common case this is an improvement, I'm ok merging it for now. But I definitely think it could be better.

Could you open an issue listing the improvements you have in mind then? That could be a nice first issue for newcomers too I think.

I'm not sure they're clear enough in my mind to suggest concrete improvements. I think we should try this for now and if I find something that bugs me, go back and fix it once I have a better idea what I would like it to be.

jyn514 · 2020-11-28T18:36:02Z

@GuillaumeGomez other than my concerns (which I don't think should block this and can be fixed later), is this waiting on anything?

GuillaumeGomez · 2020-11-28T18:42:33Z

I don't think so. Next improvements will come when we'll have more tests added I guess.

jyn514 · 2020-11-28T19:09:51Z

@bors r+

bors · 2020-11-28T19:09:53Z

📌 Commit fa14f22 has been approved by jyn514

…rror-output, r=jyn514 Improve rustdoc JS tests error output It's pretty common when starting to add new tests for rustdoc-js to have issues to understand the errors. With this, it should make things a bit simpler. So now, in case of an error, it displays: ``` ---- [js-doc-test] rustdoc-js/basic.rs stdout ---- error: rustdoc-js test failed! failed to decode compiler output as json: line: { output: Checking "basic" ... FAILED ==> Result not found in 'others': '{"path":"basic","name":"Fo"}' Diff of first error: { "path": "basic", - "name": "Fo", + "name": "Foo", } thread '[js-doc-test] rustdoc-js/basic.rs' panicked at 'explicit panic', src/tools/compiletest/src/json.rs:126:21 ``` I think it was `@camelid` who asked about it a few days ago? r? `@jyn514`

Rollup of 11 pull requests Successful merges: - rust-lang#79327 (Require allocator to be static for boxed `Pin`-API) - rust-lang#79340 (Rename "stability" CSS class to "item-info" and combine `document_stability` with `document_short`) - rust-lang#79363 (BTreeMap: try to enhance various comments) - rust-lang#79395 (Move ui if tests from top-level into `expr/if`) - rust-lang#79443 (Improve rustdoc JS tests error output) - rust-lang#79464 (Extend doc keyword feature by allowing any ident) - rust-lang#79484 (add enable-full-tools to freebsd builds to prevent occasional link er…) - rust-lang#79505 (Cleanup: shorter and faster code) - rust-lang#79514 (Add test for issue rust-lang#54121: order dependent trait bounds) - rust-lang#79516 (Remove unnecessary `mut` binding) - rust-lang#79528 (Fix a bootstrap comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jyn514 Nov 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2020

jyn514 added A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-testsuite Area: The testsuite used to check the correctness of rustc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 26, 2020

camelid reviewed Nov 26, 2020

View reviewed changes

jyn514 approved these changes Nov 26, 2020

View reviewed changes

src/tools/rustdoc-js/tester.js Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the improve-rustdoc-js-error-output branch from c439b42 to 364868f Compare November 26, 2020 20:34

jyn514 reviewed Nov 26, 2020

View reviewed changes

Improve rustdoc JS tests error output

fa14f22

GuillaumeGomez force-pushed the improve-rustdoc-js-error-output branch from 364868f to fa14f22 Compare November 26, 2020 20:59

jyn514 reviewed Nov 27, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2020

GuillaumeGomez mentioned this pull request Nov 28, 2020

Rollup of 5 pull requests #79515

Closed

Dylan-DPC-zz mentioned this pull request Nov 29, 2020

Rollup of 11 pull requests #79529

Merged

bors merged commit d5d6036 into rust-lang:master Nov 29, 2020

rustbot added this to the 1.50.0 milestone Nov 29, 2020

GuillaumeGomez deleted the improve-rustdoc-js-error-output branch November 29, 2020 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rustdoc JS tests error output #79443

Improve rustdoc JS tests error output #79443

GuillaumeGomez commented Nov 26, 2020

camelid Nov 26, 2020

GuillaumeGomez Nov 26, 2020

GuillaumeGomez Nov 26, 2020

jyn514 Nov 26, 2020

GuillaumeGomez Nov 26, 2020

GuillaumeGomez Nov 26, 2020

GuillaumeGomez Nov 26, 2020

camelid Nov 27, 2020 •

edited

Loading

jyn514 left a comment

GuillaumeGomez commented Nov 26, 2020

jyn514 Nov 26, 2020

GuillaumeGomez Nov 26, 2020

jyn514 Nov 26, 2020

GuillaumeGomez Nov 26, 2020

GuillaumeGomez Nov 26, 2020

jyn514 Nov 26, 2020

GuillaumeGomez Nov 27, 2020

GuillaumeGomez Nov 27, 2020

GuillaumeGomez commented Nov 26, 2020

camelid commented Nov 26, 2020

GuillaumeGomez commented Nov 27, 2020

jyn514 Nov 27, 2020 •

edited

Loading

camelid Nov 27, 2020

jyn514 Nov 27, 2020

GuillaumeGomez Nov 27, 2020

jyn514 Nov 27, 2020

GuillaumeGomez Nov 27, 2020

jyn514 Nov 27, 2020

GuillaumeGomez Nov 27, 2020

jyn514 Nov 28, 2020

jyn514 commented Nov 28, 2020

GuillaumeGomez commented Nov 28, 2020

jyn514 commented Nov 28, 2020

bors commented Nov 28, 2020

	output += ' ' + contentToDiffLine(key, value) + '\n';
	output += ' ' + contentToDiffLine(key, value) + '\n';

Improve rustdoc JS tests error output #79443

Improve rustdoc JS tests error output #79443

Conversation

GuillaumeGomez commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 26, 2020

camelid commented Nov 26, 2020

GuillaumeGomez commented Nov 27, 2020

jyn514 Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Nov 28, 2020

GuillaumeGomez commented Nov 28, 2020

jyn514 commented Nov 28, 2020

bors commented Nov 28, 2020

camelid Nov 27, 2020 •

edited

Loading

jyn514 Nov 27, 2020 •

edited

Loading