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

Allow excluding cache based on status code #1403

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Allow excluding cache based on status code #1403

wants to merge 7 commits into from

Conversation

dmathieu
Copy link

Closes #1400

This introduces an option --cache-exclude-status, which allows specifying a range of HTTP status codes which will be ignored from the cache.

dmathieu
dmathieu commented Apr 11, 2024
View reviewed changes
lychee-bin/src/options.rs Outdated Show resolved Hide resolved
lychee-bin/src/commands/check.rs Outdated Show resolved Hide resolved
@dmathieu dmathieu marked this pull request as ready for review April 11, 2024 12:55
@mre
Copy link
Member

mre commented Apr 12, 2024

Perfect. Great work!

Perhaps the only thing I'd add is an integration test, similar to this:

lychee/lychee-bin/tests/cli.rs

Lines 794 to 858 in eff77d6

#[tokio::test]
async fn test_lycheecache_file() -> Result<()> {
let base_path = fixtures_path().join("cache");
let cache_file = base_path.join(LYCHEE_CACHE_FILE);
// Unconditionally remove cache file if it exists
let _ = fs::remove_file(&cache_file);
let mock_server_ok = mock_server!(StatusCode::OK);
let mock_server_err = mock_server!(StatusCode::NOT_FOUND);
let mock_server_exclude = mock_server!(StatusCode::OK);
let dir = tempfile::tempdir()?;
let mut file = File::create(dir.path().join("c.md"))?;
writeln!(file, "{}", mock_server_ok.uri().as_str())?;
writeln!(file, "{}", mock_server_err.uri().as_str())?;
writeln!(file, "{}", mock_server_exclude.uri().as_str())?;
let mut cmd = main_command();
let test_cmd = cmd
.current_dir(&base_path)
.arg(dir.path().join("c.md"))
.arg("--verbose")
.arg("--no-progress")
.arg("--cache")
.arg("--exclude")
.arg(mock_server_exclude.uri());
assert!(
!cache_file.exists(),
"cache file should not exist before this test"
);
// run first without cache to generate the cache file
test_cmd
.assert()
.stderr(contains(format!("[200] {}/\n", mock_server_ok.uri())))
.stderr(contains(format!(
"[404] {}/ | Failed: Network error: Not Found\n",
mock_server_err.uri()
)));
// check content of cache file
let data = fs::read_to_string(&cache_file)?;
assert!(data.contains(&format!("{}/,200", mock_server_ok.uri())));
assert!(data.contains(&format!("{}/,404", mock_server_err.uri())));
// run again to verify cache behavior
test_cmd
.assert()
.stderr(contains(format!(
"[200] {}/ | Cached: OK (cached)\n",
mock_server_ok.uri()
)))
.stderr(contains(format!(
"[404] {}/ | Cached: Error (cached)\n",
mock_server_err.uri()
)));
// clear the cache file
fs::remove_file(&cache_file)?;
Ok(())
}

Probably it's just a matter of copy-pasting that block and changing the parameter, to test the new flag.
Would you be interested in adding that?
I could understand if it's outside the scope for now, though, and I'd be fine with merging it like it is as well. Let me know what you prefer.

@dmathieu
Copy link
Author

I'm happy to do it, but it'll have to wait for a few days. I can then do it either in this PR, or in a new one.

@mre
Copy link
Member

mre commented Apr 15, 2024

Just tested the changes locally.
When I run lychee like below, the cache is empty:

❯❯❯ cargo run -- --verbose --cache https://lychee.cli.rs/
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s
     Running `target/debug/lychee --verbose --cache 'https://lychee.cli.rs/'`
[INFO ] Cache is recent (age: 1m 6s, max age: 1d 0h 0m 0s). Using.
✔ [200] https://lychee.cli.rs/introduction/
✔ [200] https://lychee.cli.rs/#_top
✔ [200] https://lychee.cli.rs/favicon.svg
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/_astro/logo.BJx6koUn.svg
✔ [200] https://lychee.cli.rs/sitemap-index.xml
✔ [200] https://lychee.cli.rs/_astro/index.fVW1leCO.css
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/_astro/logo.BJx6koUn_1qY8Fi.svg
✔ [200] https://github.com/lycheeverse/lycheeverse.github.io/edit/master/src/content/docs/index.mdx
✔ [200] https://github.com/lycheeverse/lychee/

🔍 12 Total (in 1s) ✅ 12 OK 🚫 0 Errors
~/C/p/l/lychee ❯❯❯ echo $?
0
~/C/p/l/lychee ❯❯❯ cat .lycheecache
~/C/p/l/lychee ❯❯❯

@mre
Copy link
Member

mre commented Apr 15, 2024

I've added the integration test we discussed, and that one seems to work. Something isn't adding up here. 🤔

@dmathieu
Copy link
Author

dmathieu commented Apr 22, 2024 •
edited

Could that be related to a cache file already existing? (in which case, the issue could be that in the case of running tests with an existing cache, the file isn't written again?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mre mre mre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow option to ignore failures in cache
2 participants
@dmathieu @mre