Fix failing tests

tests/fixtures/mod.rs

@@ -4,7 +4,9 @@ use assert_fs::prelude::*;
 use port_check::free_local_port;
 use reqwest::Url;
 use rstest::fixture;
+use std::path::Path;
 use std::process::{Child, Command, Stdio};
+use std::sync::LazyLock;
 use std::thread::sleep;
 use std::time::{Duration, Instant};
 
@@ -43,13 +45,24 @@ pub static DIRECTORIES: &[&str] = &["dira/", "dirb/", "dirc/"];
 pub static HIDDEN_DIRECTORIES: &[&str] = &[".hidden_dir1/", ".hidden_dir2/"];
 
 /// Files nested at different levels under the same root directory
+///
+/// This is not a `&[&str]` because in percent-encoding, path segments and full paths
+/// are encoded differently.
 #[allow(dead_code)]
-pub static NESTED_FILES_UNDER_SINGLE_ROOT: &[&str] =
-    &["someDir/alpha", "someDir/some_sub_dir/bravo"];
-
-/// Name of a deeply nested file
+pub static NESTED_FILES_UNDER_SINGLE_ROOT: LazyLock<Vec<&Path>> = LazyLock::new(|| {
+    vec![
+        Path::new("someDir/alpha"),
+        Path::new("someDir/some_sub_dir/bravo"),
+    ]
+});
+
+/// Path to a deeply nested file
+///
+/// This is not a `&str` because in percent-encoding, path segments and full paths
+/// are encoded differently.
 #[allow(dead_code)]
-pub static DEEPLY_NESTED_FILE: &str = "very/deeply/nested/test.rs";
+pub static DEEPLY_NESTED_FILE: LazyLock<&Path> =
+    LazyLock::new(|| Path::new("very/deeply/nested/test.rs"));
 
 /// Test fixture which creates a temporary directory with a few files and directories inside.
 /// The directories also contain files.
@@ -77,8 +90,8 @@ pub fn tmpdir() -> TempDir {
         }
     }
 
-    let mut nested_files = NESTED_FILES_UNDER_SINGLE_ROOT.to_vec();
-    nested_files.push(DEEPLY_NESTED_FILE);
+    let mut nested_files = NESTED_FILES_UNDER_SINGLE_ROOT.clone();
+    nested_files.push(&DEEPLY_NESTED_FILE);
     for file in nested_files {
         tmpdir
             .child(file)

tests/navigation.rs

@@ -5,6 +5,7 @@ use fixtures::{server, Error, TestServer, DEEPLY_NESTED_FILE, DIRECTORIES};
 use pretty_assertions::{assert_eq, assert_ne};
 use rstest::rstest;
 use select::document::Document;
+use std::path::Component;
 use std::process::{Command, Stdio};
 use utils::get_link_from_text;
 use utils::get_link_hrefs_with_prefix;
@@ -69,16 +70,19 @@ fn can_navigate_into_dirs_and_back(server: TestServer) -> Result<(), Error> {
 #[rstest]
 /// We can navigate deep into the file tree and back using shown links.
 fn can_navigate_deep_into_dirs_and_back(server: TestServer) -> Result<(), Error> {
-    // Create a vector of directory names. We don't need to fetch the file and so we'll
-    // remove that part.
-    let dir_names = {
-        let mut comps = DEEPLY_NESTED_FILE
-            .split('/')
-            .map(|d| format!("{d}/"))
-            .collect::<Vec<String>>();
-        comps.pop();
-        comps
-    };
+    // Create a vector of parent directory names.
+    let dir_names = DEEPLY_NESTED_FILE
+        .parent()
+        .unwrap()
+        .components()
+        .map(|comp| {
+            let Component::Normal(dir) = comp else {
+                unreachable!()
+            };
+            dir.to_str().unwrap()
+        })
+        .collect::<Vec<_>>();
+    dbg!(&dir_names);
     let base_url = server.url();
 
     // First we'll go forwards through the directory tree and then we'll go backwards.
@@ -88,7 +92,8 @@ fn can_navigate_deep_into_dirs_and_back(server: TestServer) -> Result<(), Error>
         let resp = reqwest::blocking::get(next_url.as_str())?;
         let body = resp.error_for_status()?;
         let parsed = Document::from_read(body)?;
-        let dir_elem = get_link_from_text(&parsed, dir_name).expect("Dir not found.");
+        let dir_elem =
+            get_link_from_text(&parsed, &format!("{dir_name}/")).expect("Dir not found.");
         next_url = next_url.join(&dir_elem)?;
     }
     assert_ne!(base_url, next_url);
@@ -115,19 +120,10 @@ fn can_navigate_using_breadcrumbs(
     #[case] server: TestServer,
     #[case] title_name: String,
 ) -> Result<(), Error> {
-    // Create a vector of directory names. We don't need to fetch the file and so we'll
-    // remove that part.
-    let dir: String = {
-        let mut comps = DEEPLY_NESTED_FILE
-            .split('/')
-            .map(|d| format!("{d}/"))
-            .collect::<Vec<String>>();
-        comps.pop();
-        comps.join("")
-    };
+    let dir = DEEPLY_NESTED_FILE.parent().unwrap().to_str().unwrap();
 
     let base_url = server.url();
-    let nested_url = base_url.join(&dir)?;
+    let nested_url = base_url.join(dir)?;
 
     let resp = reqwest::blocking::get(nested_url.as_str())?;
     let body = resp.error_for_status()?;

tests/rm_files.rs

@@ -3,36 +3,69 @@ mod fixtures;
 use anyhow::bail;
 use assert_fs::fixture::TempDir;
 use fixtures::{server, server_no_stderr, tmpdir, TestServer};
+use percent_encoding::utf8_percent_encode;
 use reqwest::blocking::Client;
 use reqwest::StatusCode;
 use rstest::rstest;
-use std::path::Path;
+use std::{
+    iter,
+    path::{Component, Path},
+};
 use url::Url;
 
 use crate::fixtures::{
     DEEPLY_NESTED_FILE, DIRECTORIES, FILES, HIDDEN_DIRECTORIES, HIDDEN_FILES,
     NESTED_FILES_UNDER_SINGLE_ROOT,
 };
 
-fn assert_rm_ok(base_url: Url, paths: &[impl AsRef<str>]) -> anyhow::Result<()> {
+include!(concat!(
+    env!("CARGO_MANIFEST_DIR"),
+    "/",
+    "src/path_utils.rs"
+));
+
+/// Construct a path for a GET request,
+/// with each path component being separately encoded.
+fn make_get_path(unencoded_path: impl AsRef<Path>) -> String {
+    unencoded_path
+        .as_ref()
+        .components()
+        .map(|comp| match comp {
+            Component::Prefix(_) | Component::RootDir => unreachable!("Not currently used"),
+            Component::CurDir => ".",
+            Component::ParentDir => "..",
+            Component::Normal(comp) => comp.to_str().unwrap(),
+        })
+        .map(|comp| utf8_percent_encode(comp, percent_encode_sets::COMPONENT).to_string())
+        .collect::<Vec<_>>()
+        .join("/")
+}
+
+/// Construct a path for a deletion POST request without any further encoding.
+///
+/// This should be kept consistent with implementation.
+fn make_del_path(unencoded_path: impl AsRef<Path>) -> String {
+    format!("rm?path=/{}", make_get_path(unencoded_path))
+}
+
+fn assert_rm_ok(base_url: Url, unencoded_paths: &[impl AsRef<Path>]) -> anyhow::Result<()> {
     let client = Client::new();
 
-    for path in paths.iter().map(AsRef::as_ref) {
+    for file_path in unencoded_paths.iter().map(AsRef::as_ref) {
+        // encode
+        let get_url = base_url.join(&make_get_path(file_path))?;
+        let del_url = base_url.join(&make_del_path(file_path))?;
+        println!("===== {file_path:?} =====");
+        println!("{get_url}, {del_url}");
+
         // check path exists
-        let _get_res = client
-            .get(base_url.join(path)?)
-            .send()?
-            .error_for_status()?;
+        let _get_res = client.get(get_url.clone()).send()?.error_for_status()?;
 
         // delete
-        let req_path = format!("rm?path=/{path}");
-        let _del_res = client
-            .post(base_url.join(&req_path)?)
-            .send()?
-            .error_for_status()?;
+        let _del_res = client.post(del_url).send()?.error_for_status()?;
 
         // check path is gone
-        let get_res = client.get(base_url.join(path)?).send()?;
+        let get_res = client.get(get_url).send()?;
         if get_res.status() != StatusCode::NOT_FOUND {
             bail!("Unexpected status code: {}", get_res.status());
         }
@@ -45,33 +78,32 @@ fn assert_rm_ok(base_url: Url, paths: &[impl AsRef<str>]) -> anyhow::Result<()>
 /// the deletion attempt in case these paths should be inaccessible via GET.
 fn assert_rm_err(
     base_url: Url,
-    paths: &[impl AsRef<str>],
+    unencoded_paths: &[impl AsRef<Path>],
     check_paths_exist: bool,
 ) -> anyhow::Result<()> {
     let client = Client::new();
 
-    for path in paths.iter().map(AsRef::as_ref) {
+    for file_path in unencoded_paths.iter().map(AsRef::as_ref) {
+        // encode
+        let get_url = base_url.join(&make_get_path(file_path))?;
+        let del_url = base_url.join(&make_del_path(file_path))?;
+        println!("===== {file_path:?} =====");
+        println!("{get_url}, {del_url}");
+
         // check path exists
         if check_paths_exist {
-            let _get_res = client
-                .get(base_url.join(path)?)
-                .send()?
-                .error_for_status()?;
+            let _get_res = client.get(get_url.clone()).send()?.error_for_status()?;
         }
 
         // delete
-        let req_path = format!("rm?path=/{path}");
-        let del_res = client.post(base_url.join(&req_path)?).send()?;
+        let del_res = client.post(del_url).send()?;
         if !del_res.status().is_client_error() {
             bail!("Unexpected status code: {}", del_res.status());
         }
 
         // check path still exists
         if check_paths_exist {
-            let _get_res = client
-                .get(base_url.join(path)?)
-                .send()?
-                .error_for_status()?;
+            let _get_res = client.get(get_url).send()?.error_for_status()?;
         }
     }
 
@@ -80,26 +112,35 @@ fn assert_rm_err(
 
 #[rstest]
 fn rm_disabled_by_default(server: TestServer) -> anyhow::Result<()> {
-    assert_rm_err(
-        server.url(),
-        &[
-            FILES,
-            HIDDEN_FILES,
-            DIRECTORIES,
-            HIDDEN_DIRECTORIES,
-            &[DEEPLY_NESTED_FILE],
-        ]
-        .concat(),
-        true,
-    )
+    let paths = [FILES, DIRECTORIES]
+        .concat()
+        .into_iter()
+        .map(Path::new)
+        .chain(iter::once(DEEPLY_NESTED_FILE.as_ref()))
+        .collect::<Vec<_>>();
+    assert_rm_err(server.url(), &paths, true)
+}
+
+#[rstest]
+fn rm_disabled_by_default_with_hidden(#[with(&["-H"])] server: TestServer) -> anyhow::Result<()> {
+    let paths = [FILES, HIDDEN_FILES, DIRECTORIES, HIDDEN_DIRECTORIES]
+        .concat()
+        .into_iter()
+        .map(Path::new)
+        .chain(iter::once(DEEPLY_NESTED_FILE.as_ref()))
+        .collect::<Vec<_>>();
+    assert_rm_err(server.url(), &paths, true)
 }
 
 #[rstest]
 fn rm_works(#[with(&["-R"])] server: TestServer) -> anyhow::Result<()> {
-    assert_rm_ok(
-        server.url(),
-        &[FILES, DIRECTORIES, &[DEEPLY_NESTED_FILE]].concat(),
-    )
+    let paths = [FILES, DIRECTORIES]
+        .concat()
+        .into_iter()
+        .map(Path::new)
+        .chain(iter::once(DEEPLY_NESTED_FILE.as_ref()))
+        .collect::<Vec<_>>();
+    assert_rm_ok(server.url(), &paths)
 }
 
 #[rstest]
@@ -129,7 +170,7 @@ fn can_rm_hidden_when_allowed(
 #[case(server_no_stderr(&["-R", "someOtherDir"]))]
 #[case(server_no_stderr(&["-R", "someDir/some_other_sub_dir"]))]
 fn rm_is_restricted(#[case] server: TestServer) -> anyhow::Result<()> {
-    assert_rm_err(server.url(), NESTED_FILES_UNDER_SINGLE_ROOT, true)
+    assert_rm_err(server.url(), &NESTED_FILES_UNDER_SINGLE_ROOT, true)
 }
 
 /// This test runs the server with --allowed-rm-dir argument and checks that
@@ -177,17 +218,17 @@ fn rm_from_symlinked_dir(
     use std::os::windows::fs::symlink_dir;
 
     // create symlink
-    const LINK_NAME: &str = "linked";
-    symlink_dir(target.path(), server.path().join(LINK_NAME))?;
+    let link: &Path = Path::new("linked");
+    symlink_dir(target.path(), server.path().join(link))?;
 
-    let files_through_link = &[FILES, DIRECTORIES]
+    let files_through_link = [FILES, DIRECTORIES]
         .concat()
         .iter()
-        .map(|name| format!("{LINK_NAME}/{name}"))
+        .map(|name| link.join(name))
         .collect::<Vec<_>>();
     if should_succeed {
-        assert_rm_ok(server.url(), files_through_link)
+        assert_rm_ok(server.url(), &files_through_link)
     } else {
-        assert_rm_err(server.url(), files_through_link, false)
+        assert_rm_err(server.url(), &files_through_link, false)
     }
 }