Skip to content
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

Improve rustdoc JS tests error output #79443

Merged

Conversation

GuillaumeGomez
Copy link
Member

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2020
@jyn514 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
output += ' + ' + contentToDiffLine(key, value) + '\n';
error = true;
} else {
output += ' ' + contentToDiffLine(key, value) + '\n';
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove the two whitespaces instead. ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

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. :-/

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

With the new output:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@camelid camelid Nov 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

LGTM modulo Camelid's nit and my suggestion.

src/tools/rustdoc-js/tester.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated!

Comment on lines 199 to 201
function betterLookingDiff(entry, data) {
let output = '{\n';
data = data[0];
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum no, not gonna work either haha.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

@camelid
Copy link
Member

camelid commented Nov 26, 2020

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
Copy link
Member Author

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

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

Comment on lines +308 to +312
// 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;
}
Copy link
Member

@jyn514 jyn514 Nov 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

jyn514 commented Nov 28, 2020

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

@GuillaumeGomez
Copy link
Member Author

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

@jyn514
Copy link
Member

jyn514 commented Nov 28, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2020

📌 Commit fa14f22 has been approved by jyn514

@bors 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 added a commit to GuillaumeGomez/rust that referenced this pull request Nov 28, 2020
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2020
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
@bors bors merged commit d5d6036 into rust-lang:master Nov 29, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 29, 2020
@GuillaumeGomez GuillaumeGomez deleted the improve-rustdoc-js-error-output branch November 29, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants