-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
tests: add markdown link checker #11358
tests: add markdown link checker #11358
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Looks like we are blocked on tcort/markdown-link-check#94 |
I like their solution: JabRef/jabref#6695 Would be good to move this to a once-a-week check. Could you change the action to be a cron instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like their solution: JabRef/jabref#6695
Would be good to move this to a once-a-week check. Could you change the action to be a cron instead?
Sure, will do.
.github/workflows/ci.yml
Outdated
@@ -172,3 +172,14 @@ jobs: | |||
|
|||
- name: Run smoke tests | |||
run: yarn smoke --debug -j=1 --retries=2 dbw oopif offline lantern metrics | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not the place to be, I can move this to a separate .yml file.
.github/workflows/ci.yml
Outdated
use-quiet-mode: 'yes' | ||
use-verbose-mode: 'yes' | ||
config-file: 'markdown.links.config.json' | ||
folder-path: 'docs/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are more files to check, beside the one from docs, we can check them. For all markdown files in the repo, it takes like 11 minutes, and some of them will be marked as 429, since this library has no throttling.
markdown.links.config.json
Outdated
{ | ||
"ignorePatterns": [ | ||
{ | ||
"pattern": "^http://www.tmeter.ru" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TMeter seems to be down, we can remove the link or replace TMeter with something else.
markdown.links.config.json
Outdated
"pattern": "^http://www.tmeter.ru" | ||
}, | ||
{ | ||
"pattern": "https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url is just fine, but markdown-link-check marks it with 4xx response code. I'm gonna check this with them, and after a fix will be available, we can remove this.
@@ -133,7 +133,7 @@ $ lighthouse --port=9222 --emulated-form-factor=none --throttling.cpuSlowdownMul | |||
|
|||
## Lighthouse as trace processor | |||
|
|||
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options]((https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)). | |||
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options](https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that a wrongly opened parenthesis was messing up the link. This link was not even transformed by github: https://github.com/GoogleChrome/lighthouse/blob/master/docs/readme.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! this is exactly the kind of thing I wanted to catch.
markdown-link-check needs an option to retry on 429 codes, but it will not help without this dependency update: link-check |
So I've managed to find a solution: Once the 429 issue will be resolved, I will come back with another pull request, so maybe we can check everything. https://github.com/andreizet/lighthouse/runs/1070765498?check_suite_focus=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this runs so infrequently, and we've seen it handle all the links, we should be good to merge this without any more changes upstream.
Worse case, we get an email some Mondays with a couple "429s"..
Thanks for taking care of this for us!
This was assigned to @paulirish so I'll defer to him.
@@ -133,7 +133,7 @@ $ lighthouse --port=9222 --emulated-form-factor=none --throttling.cpuSlowdownMul | |||
|
|||
## Lighthouse as trace processor | |||
|
|||
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options]((https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)). | |||
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options](https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! this is exactly the kind of thing I wanted to catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i also wanted that config file to move out of root. happy that's done. lg!
thanks for working on this @andreizet !
Summary
This is a build related change, it adds a new action that checks for broken links inside markdown files when performing the CI actions.
It will prevent issues like #11089
Related Issues/PRs