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

docs: Initial set of documentation tests #567

Merged
merged 1 commit into from
Jan 25, 2021
Merged

docs: Initial set of documentation tests #567

merged 1 commit into from
Jan 25, 2021

Conversation

kevgo
Copy link
Contributor

@kevgo kevgo commented Jul 10, 2020

Proposed changes

Checks technical parts of the documentation using a documentation testing framework (Text-Runner) and fixes a number of issues found.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

Keeping it simple for now to provide an idea of the capabilities of the tool. In later stages we can automate larger tutorials as well.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
```shell script
make resetdb
make test-resetdb
Copy link
Contributor Author

@kevgo kevgo Jul 10, 2020

Choose a reason for hiding this comment

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

This is a documentation bug found by Text-Runner. make resetdb doesn't exist. Is my fix to make test-resetdb correct?

Makefile Outdated Show resolved Hide resolved
@kevgo kevgo changed the title Initial set of documentation tests docs: Initial set of documentation tests Jul 10, 2020
@kevgo
Copy link
Contributor Author

kevgo commented Jul 10, 2020

@tacurran @aeneasr you can see more details when running text-run --offline --format detailed README.md. Please hit me up if you want me to walk you through this.

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Nice! This will help a lot in keeping the docs tidy!

One question I have is regarding the runenr files - it looks like these are pretty generic (e.g. "is file executable?"). Instead of copying them per repository (and subsequently maintaining them in all repos), would it be possible to load them from a stdlib or a remote or something?

@kevgo
Copy link
Contributor Author

kevgo commented Jul 13, 2020

You hit the nail on the head: sharing actions is the last big missing feature for Text-Runner. There is a stdlib of Text-Runner actions here. But many actions are too specific to be in a global stdlib. In kevgo/text-runner#896 I'm thinking about a way to load them from NPM modules. Do you have an idea how Text-Runner could discover which NPM packages listed in package.json contain Text-Runner actions?

@aeneasr
Copy link
Member

aeneasr commented Jul 14, 2020

I would add the stdlibs to the core package. For additional things, how about

<span data-textrunner="@textrunner/mypkg"/>

which translates to

const runner = require('@textrunner/mypkg")

?

@kevgo
Copy link
Contributor Author

kevgo commented Jul 14, 2020

Your idea to make the module name a part of the action name is brilliant! This is not only straightforward to implement, it also makes clear where a particular action comes from when debugging documentation. Thanks!!

Btw stdlib is already built into Text-Runner.

@aeneasr
Copy link
Member

aeneasr commented Jul 15, 2020

Awesome! Should we wait for upstream #896 then before merge or merge right away? If we merge right away, I think it would make sense to put this into ory/meta and build automation around synching this for all the big 4 (kratos, oathkeeper, keto, hydra). What you think?

@kevgo
Copy link
Contributor Author

kevgo commented Jul 15, 2020

It will be pretty straightforward to finish #896. I just have to find time to get to it. The latest this weekend. Let's not invest any energy into a meta setup that will be obsolete next week. Probably better to just wait.

@aeneasr
Copy link
Member

aeneasr commented Jul 16, 2020

Nice :)

@aeneasr aeneasr marked this pull request as draft July 16, 2020 09:27
@aeneasr aeneasr added the stale Feedback from one or more authors is required to proceed. label Jul 27, 2020
This was referenced Oct 6, 2020
Copy link
Contributor Author

@kevgo kevgo left a comment

Choose a reason for hiding this comment

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

Please have another look. This is a small set of initial documentation tests, with a lot more possible later. I have left some comments outlining how it works.

Thanks again for your very helpful suggestions. I have finished implementing them. Since they required breaking changes on Text-Runner, I had to implement a backlog of other planned changes in order to release Text-Runner 5.0. It ended up being quite a large release and a large-scale refactor that kept me busy until now: https://github.com/kevgo/text-runner/blob/master/CHANGELOG.md#500

<a href="#backers" alt="sponsors on Open Collective"><img src="https://opencollective.com/ory/backers/badge.svg" /></a>
<a href="#sponsors" alt="Sponsors on Open Collective"><img src="https://opencollective.com/ory/sponsors/badge.svg" /></a>
<a href="https://opencollective.com/ory" alt="sponsors on Open Collective"><img src="https://opencollective.com/ory/backers/badge.svg" /></a>
<a href="https://opencollective.com/ory" alt="Sponsors on Open Collective"><img src="https://opencollective.com/ory/sponsors/badge.svg" /></a>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These targets were broken. I didn't find any targets for backers and sponsors on opencollective.com, so I link to that page in both cases instead. Please let me know if there are better URLs.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this moved to #who-is-using-it

@@ -187,7 +187,7 @@ that your company deserves a spot here, reach out to

We also want to thank all individual contributors

<img src="https://opencollective.com/ory/contributors.svg?width=890&button=false" /></a>
<img src="https://opencollective.com/ory/contributors.svg?width=890&button=false" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Text-Runner found this orphaned tag.

Comment on lines +335 to +342
Run <code type="shell/command">kratos -h</code> or
<code type="shell/command">kratos help</code>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you describe shell commands to get command-line help. I tag them as shell commands. Text-Runner executes shell commands in a subshell and verifies exit code 0.

If somebody (hypothetically) changes the Kratos source so that the CLI argument is kratos --help, Text-Runner will enforce that this documentation gets updated as well!

These shell commands only succeed if somebody runs make install first to create the Kratos binary. This could be done by other tests (end-to-end tests?) that run before these documentation tests. Text-Runner can of course also run make install. It is currently documented below and takes a while, so I'm not doing it.

Comment on lines +354 to +362
<pre type="make/command">
make install
```
</pre>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I describe that this code block contains a make command. Text-Runner looks inside the Makefile and makes sure the documented targets exist there.

Comment on lines +430 to +438
<pre type="repo/executable">
./test/e2e/run.sh
```

</pre>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tag this block as containing an executable that exists in this repository. Text-Runner verifies that this file exists and is executable. It doesn't run it. I assume you have separate tests for it.

We could run it here, then we would have to tag it to be of type shell/command.

[Text-Runner](https://github.com/kevgo/text-runner).

- test all documentation: <code type="make/command">make test-docs</code>
- test an individual file: <code type="npm/installed-executable">text-run</code>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block of type npm/installed-executable contains an executable installed by an external npm package. Text-Runner verifies it by looking for this file inside node_modules/.bin.

package.json Outdated
Comment on lines 20 to 23
"textrun-make": "0.0.9",
"textrun-npm": "0.0.9",
"textrun-repo": "0.0.9",
"textrun-shell": "0.0.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These npm packages are a form of "standard library" for Text-Runner and contain all the code block types used in this document. Everything is reusable elsewhere.

@kevgo kevgo marked this pull request as ready for review October 14, 2020 02:28
@kevgo
Copy link
Contributor Author

kevgo commented Oct 14, 2020

The biggest change is that tags in the documentation are no longer Text-Runner specific but describe the semantics of the document content. This makes Text-Runner a lot less invasive. Knowing what type of content the different document parts contain, Text-Runner can verify that they are correct.

@zepatrik
Copy link
Member

Nice, do you have any guide on writing these tests? We could add a reference to that in our PR template: https://github.com/ory/meta/blob/master/templates/repository/server/.github/pull_request_template.md

@kevgo
Copy link
Contributor Author

kevgo commented Oct 14, 2020

@zepatrik Text-Runner is documented here. Not sure Text-Runner needs to be mentioned in the PR template itself but once we adopt it more broadly it might make sense to add it to the documentation guidelines.

@zepatrik
Copy link
Member

Right, I just thought if we add it right away people will add the textrunner annotations right away.

@aeneasr
Copy link
Member

aeneasr commented Jan 11, 2021

Hey @kevgo I wanted to check in and see what the status is for this PR! Is it ready to be merged and reviewed or does it need a bit more work? Let me know!

@kevgo
Copy link
Contributor Author

kevgo commented Jan 11, 2021

Hey @aeneasr sorry for the miscommunication, this PR was ready for 👀 since October. Should I tag you next time?

This PR makes Text-Runner work (run via make test-docs) and fixes various identified documentation errors. Not sure if the documentation fixes are in the correct place or if the README.md file is auto-generated.

The only thing left is running the doc tests on CircleCI. They need the Kratos binary and a Node runtime. The way I typically do this is (step 1) compile the binary in a Go container and save the binary via persist_to_workspace, then (step 2) copy that binary into various containers for unit, end-to-end, and documentation tests (via attach_workspace) and run those in parallel. Please guide me if and how you want to do it here. Thanks!

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Addressed your questions regarding CircleCI. For the CircleCI task itself, you could probably add this to the test pipeline over here like so:

      -
        run: make test-docs

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -114,6 +114,10 @@ format: .bin/goimports
docker:
docker build -f .docker/Dockerfile-build -t oryd/kratos:latest-sqlite .

# Runs the documentation tests
test-docs:
text-run README.md
Copy link
Member

Choose a reason for hiding this comment

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

If this is instead npm run text-run and text-run is a package.json run script, then this should already work in the CI!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions! I implemented all of them but it doesn't work on CI.

  • When I run this inside the test job (as suggested by you), it doesn't find npm. I assume this job runs inside a Go container.
  • When I run this inside the validate job, it doesn't have the Kratos binary.

I think the validate job seems a good place to run this. Do you have any pointers how to get the Kratos binary into that container in a way that is idiomatic with your setup?

Copy link
Member

@aeneasr aeneasr Jan 14, 2021

Choose a reason for hiding this comment

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

Ah ok, I should have seen that when writing my comment. The default go image does not have NodeJS, but there is one which does:

    docker:
      - image: cimg/go:1.15-node

Replacing https://github.com/ory/kratos/pull/567/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R16 with that image should then make this work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer! For future reference, the cimg/go:1.15-node image has a folder structure that is incompatible with the folder structure CircleCI expects. This causes the code checkout step to fail. The circleci/golang:1.15-node image doesn't have this problem and seems to work well.

@aeneasr aeneasr added docs feat New feature or request. and removed stale Feedback from one or more authors is required to proceed. labels Jan 13, 2021
@aeneasr aeneasr self-assigned this Jan 13, 2021
@aeneasr aeneasr added this to the v0.6.0-alpha.1 milestone Jan 13, 2021
@kevgo
Copy link
Contributor Author

kevgo commented Jan 20, 2021

@aeneasr alright I figured it out. I'm using the circleci/golang:1.15-node Docker image, which is already used in the validate job. Have to compile Kratos manually to make it accessible in the test job. Please take another look.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you so much @kevgo !

@aeneasr aeneasr merged commit c30eb26 into ory:master Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants