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.

Sign up for GitHub

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

Jump to bottom

Jsondocck improvements #82311

Merged
merged 6 commits into from
Feb 23, 2021
Merged

Jsondocck improvements #82311

merged 6 commits into from
Feb 23, 2021

Conversation

aDotInTheVoid
Copy link
Member

@aDotInTheVoid aDotInTheVoid commented Feb 20, 2021 •
edited by jyn514
Loading

Adds 2 new commands, @is and @set.

@is works like @has, except instead of checking if any value matches, it checks that there is exactly one value, and it matches. This allows more precise testing.

@set gets a value, and saves it to be used later. This makes it possible to check that an item appears in the correct module.

Once this lands, the rest of the test suite can be upgraded to use these.

cc @CraftSpider

@rustbot modify labels: +T-rustdoc +A-rustdoc-json +A-testsuite

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2021
@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend 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 Feb 20, 2021
@Mark-Simulacrum
Copy link
Member

r? @jyn514

@jyn514
Copy link
Member

jyn514 commented Feb 21, 2021 •
edited
Loading

Once this lands, the rest of the test suite can be upgraded to use these.

Can you update one or two tests to use the new syntax so I can get an idea what it looks like? Nevermind, you did this already, thanks.

jyn514
jyn514 reviewed Feb 21, 2021
View reviewed changes
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.

Can you post an example of what the error looks like if a @set command is written incorrectly or an @is command is false?

src/tools/jsondocck/src/main.rs Outdated Show resolved Hide resolved
src/tools/jsondocck/src/main.rs Outdated Show resolved Hide resolved
src/test/rustdoc-json/nested.rs
// @count - "$.index[*][?(@.name=='l3')].inner.items[*]" 1
// @set l3_id = - "$.index[*][?(@.name=='l3')].id"
// @has - "$.index[*][?(@.name=='l1')].inner.items[*]" $l3_id
Copy link
Member

Choose a reason for hiding this comment

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

$ already has a meaning in JSONPath, it means the root. I guess that shouldn't overlap here because you're testing a JSON value, it's not part of the path syntax? What happens if you want to match a string with $ in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

$foo -> Variable foo,
"$foo" -> String $foo

Because json can only start with ", {, or [, it is unambiguous

Copy link
Member Author

Choose a reason for hiding this comment

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

But this should be documented

src/tools/jsondocck/src/main.rs Outdated Show resolved Hide resolved
src/tools/jsondocck/src/main.rs Outdated Show resolved Hide resolved
src/tools/jsondocck/src/main.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2021
@aDotInTheVoid
Copy link
Member Author

Can you post an example of what the error looks like if a @set command is written incorrectly or an @is command is false?

test

// @set var = failing.json "foo.bar.baz"
// @is failing.json root "not_root"

the relavent output

---- [rustdoc-json] rustdoc-json/reexport/failing.rs stdout ----

error: jsondocck failed!
status: exit code: 1
command: "/home/nixon/upstreams/rust/rust/build/x86_64-unknown-linux-gnu/stage0-tools-bin/jsondocck" "--doc-dir" "/home/nixon/upstreams/rust/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-json/reexport/failing" "--template" "/home/nixon/upstreams/rust/rust/src/test/rustdoc-json/reexport/failing.rs"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
Failed check: `@set var = failing.json foo.bar.baz` didn't match when it should on line 4
Failed check: `@is failing.json root not_root` didn't match when it should on line 5
Error: "Jsondocck failed for /home/nixon/upstreams/rust/rust/src/test/rustdoc-json/reexport/failing.rs"

------------------------------------------

It's the same as anything, as currently the only way we report errors is returning false (or true for !), and then an error is given with just the failing comment.

(This is not tecnicly true) as some errors are panics, eg variable reassignment, lookup of variable not in hashmap

@aDotInTheVoid
Copy link
Member Author

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2021
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2021
@aDotInTheVoid
Copy link
Member Author

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 22, 2021

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit 4c949a4 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 Feb 22, 2021
@CraftSpider
Copy link
Contributor

I know this was already approved, but +1 from me. I appreciate the ability to check both 'any match' and 'exactly 1 matches' easily

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 22, 2021
@GuillaumeGomez
Rollup merge of rust-lang#82311 - aDotInTheVoid:jsondocck-improvement…
6fedd91 
…s, r=jyn514

Jsondocck improvements

Adds 2 new commands, ``@is`` and ``@set`.`

``@is`` works like ``@has`,` except instead of checking if any value matches, it checks that there is exactly one value, and it matches. This allows more precise testing.

``@set`` gets a value, and saves it to be used later. This makes it possible to check that an item appears in the correct module.

Once this lands, the rest of the test suite can be upgraded to use these.

cc `@CraftSpider`

 `@rustbot` modify labels: +T-rustdoc +A-rustdoc-json +A-testsuite
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 22, 2021
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 23, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
@bors
Auto merge of rust-lang#82443 - Dylan-DPC:rollup-yni7uio, r=Dylan-DPC
fe1bf8e 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81629 (Point out implicit deref coercions in borrow)
 - rust-lang#82113 (Improve non_fmt_panic lint.)
 - rust-lang#82258 (Implement -Z hir-stats for nested foreign items)
 - rust-lang#82296 (Support `pub` on `macro_rules`)
 - rust-lang#82297 (Consider auto derefs before warning about write only fields)
 - rust-lang#82305 (Remove many RefCells from DocContext)
 - rust-lang#82308 (Lower condition of `if` expression before it's "then" block)
 - rust-lang#82311 (Jsondocck improvements)
 - rust-lang#82362 (Fix mir-cfg dumps)
 - rust-lang#82391 (disable atomic_max/min tests in Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 619e47b into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@aDotInTheVoid aDotInTheVoid deleted the jsondocck-improvements branch July 5, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-rustdoc-json Area: Rustdoc JSON backend 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
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aDotInTheVoid @rust-highfive @Mark-Simulacrum @jyn514 @bors @CraftSpider @rustbot