refactor(cli): generalize module specifier collection (#11679)

cli/fs_util.rs

@@ -3,6 +3,7 @@
 use deno_core::error::AnyError;
 use deno_core::error::Context;
 pub use deno_core::normalize_path;
+use deno_core::ModuleSpecifier;
 use deno_runtime::deno_crypto::rand;
 use std::env::current_dir;
 use std::fs::OpenOptions;
@@ -171,6 +172,49 @@ where
   Ok(target_files)
 }
 
+/// Collects module specifiers that satisfy the given predicate as a file path, by recursively walking `include`.
+/// Specifiers that start with http and https are left intact.
+pub fn collect_specifiers<P>(
+  include: Vec<String>,
+  predicate: P,
+) -> Result<Vec<ModuleSpecifier>, AnyError>
+where
+  P: Fn(&Path) -> bool,
+{
+  let (include_urls, include_paths): (Vec<String>, Vec<String>) =
+    include.into_iter().partition(|url| {
+      let url = url.to_lowercase();
+      url.starts_with("http://") || url.starts_with("https://")
+    });
+
+  let mut prepared = vec![];
+
+  let root_path = std::env::current_dir()?;
+  for path in include_paths {
+    let p = normalize_path(&root_path.join(path));
+    if p.is_dir() {
+      let test_files = collect_files(&[p], &[], &predicate).unwrap();
+      let mut test_files_as_urls = test_files
+        .iter()
+        .map(|f| ModuleSpecifier::from_file_path(f).unwrap())
+        .collect::<Vec<ModuleSpecifier>>();
+
+      test_files_as_urls.sort();
+      prepared.extend(test_files_as_urls);
+    } else {
+      let url = ModuleSpecifier::from_file_path(p).unwrap();
+      prepared.push(url);
+    }
+  }
+
+  for remote_url in include_urls {
+    let url = ModuleSpecifier::parse(&remote_url)?;
+    prepared.push(url);
+  }
+
+  Ok(prepared)
+}
+
 // Asynchronously removes a directory and all its descendants, but does not error
 // when the directory does not exist.
 pub async fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
@@ -328,4 +372,82 @@ mod tests {
     }
     assert_eq!(result.len(), expected.len());
   }
+
+  #[test]
+  fn test_collect_specifiers() {
+    fn create_files(dir_path: &Path, files: &[&str]) {
+      std::fs::create_dir(dir_path).expect("Failed to create directory");
+      for f in files {
+        let path = dir_path.join(f);
+        std::fs::write(path, "").expect("Failed to create file");
+      }
+    }
+
+    // dir.ts
+    // ├── a.ts
+    // ├── b.js
+    // ├── child
+    // │   ├── e.mjs
+    // │   ├── f.mjsx
+    // │   ├── .foo.TS
+    // │   └── README.md
+    // ├── c.tsx
+    // ├── d.jsx
+    // └── ignore
+    //     ├── g.d.ts
+    //     └── .gitignore
+
+    let t = TempDir::new().expect("tempdir fail");
+
+    let root_dir_path = t.path().join("dir.ts");
+    let root_dir_files = ["a.ts", "b.js", "c.tsx", "d.jsx"];
+    create_files(&root_dir_path, &root_dir_files);
+
+    let child_dir_path = root_dir_path.join("child");
+    let child_dir_files = ["e.mjs", "f.mjsx", ".foo.TS", "README.md"];
+    create_files(&child_dir_path, &child_dir_files);
+
+    let ignore_dir_path = root_dir_path.join("ignore");
+    let ignore_dir_files = ["g.d.ts", ".gitignore"];
+    create_files(&ignore_dir_path, &ignore_dir_files);
+
+    let result = collect_specifiers(
+      vec![
+        "http://localhost:8080".to_string(),
+        root_dir_path.to_str().unwrap().to_string(),
+        "https://localhost:8080".to_string(),
+      ],
+      |path| {
+        // exclude dotfiles
+        path
+          .file_name()
+          .and_then(|f| f.to_str())
+          .map_or(false, |f| !f.starts_with('.'))
+      },
+    )
+    .unwrap();
+
+    let root_dir_url = ModuleSpecifier::from_file_path(
+      canonicalize_path(&root_dir_path).unwrap(),
+    )
+    .unwrap()
+    .to_string();
+    let expected: Vec<ModuleSpecifier> = [
+      &format!("{}/a.ts", root_dir_url),
+      &format!("{}/b.js", root_dir_url),
+      &format!("{}/c.tsx", root_dir_url),
+      &format!("{}/child/README.md", root_dir_url),
+      &format!("{}/child/e.mjs", root_dir_url),
+      &format!("{}/child/f.mjsx", root_dir_url),
+      &format!("{}/d.jsx", root_dir_url),
+      &format!("{}/ignore/g.d.ts", root_dir_url),
+      "http://localhost:8080",
+      "https://localhost:8080",
+    ]
+    .iter()
+    .map(|f| ModuleSpecifier::parse(f).unwrap())
+    .collect::<Vec<ModuleSpecifier>>();
+
+    assert_eq!(result, expected);
+  }
 }

cli/main.rs

@@ -1019,7 +1019,6 @@ async fn test_command(
   let program_state = ProgramState::build(flags.clone()).await?;
 
   let include = include.unwrap_or_else(|| vec![".".to_string()]);
-  let cwd = std::env::current_dir().expect("No current directory");
 
   let permissions = Permissions::from_options(&flags.clone().into());
   let lib = if flags.unstable {
@@ -1040,15 +1039,13 @@ async fn test_command(
     // TODO(caspervonb) clean this up.
     let resolver = |changed: Option<Vec<PathBuf>>| {
       let test_modules_result = if doc {
-        test_runner::collect_test_module_specifiers(
+        fs_util::collect_specifiers(
           include.clone(),
-          &cwd,
           fs_util::is_supported_ext_test,
         )
       } else {
-        test_runner::collect_test_module_specifiers(
+        fs_util::collect_specifiers(
           include.clone(),
-          &cwd,
           tools::test_runner::is_supported,
         )
       };
@@ -1173,7 +1170,6 @@ async fn test_command(
     };
 
     let operation = |modules_to_reload: Vec<ModuleSpecifier>| {
-      let cwd = cwd.clone();
       let filter = filter.clone();
       let include = include.clone();
       let lib = lib.clone();
@@ -1182,9 +1178,8 @@ async fn test_command(
 
       async move {
         let doc_modules = if doc {
-          test_runner::collect_test_module_specifiers(
+          fs_util::collect_specifiers(
             include.clone(),
-            &cwd,
             fs_util::is_supported_ext_test,
           )?
         } else {
@@ -1197,9 +1192,8 @@ async fn test_command(
           .cloned()
           .collect();
 
-        let test_modules = test_runner::collect_test_module_specifiers(
+        let test_modules = fs_util::collect_specifiers(
           include.clone(),
-          &cwd,
           tools::test_runner::is_supported,
         )?;
 
@@ -1232,18 +1226,16 @@ async fn test_command(
     file_watcher::watch_func(resolver, operation, "Test").await?;
   } else {
     let doc_modules = if doc {
-      test_runner::collect_test_module_specifiers(
+      fs_util::collect_specifiers(
         include.clone(),
-        &cwd,
         fs_util::is_supported_ext_test,
       )?
     } else {
       Vec::new()
     };
 
-    let test_modules = test_runner::collect_test_module_specifiers(
+    let test_modules = fs_util::collect_specifiers(
       include.clone(),
-      &cwd,
       tools::test_runner::is_supported,
     )?;
 

cli/tools/test_runner.rs

@@ -5,8 +5,6 @@ use crate::ast::Location;
 use crate::colors;
 use crate::create_main_worker;
 use crate::file_fetcher::File;
-use crate::fs_util::collect_files;
-use crate::fs_util::normalize_path;
 use crate::media_type::MediaType;
 use crate::module_graph;
 use crate::program_state::ProgramState;
@@ -20,7 +18,6 @@ use deno_core::futures::FutureExt;
 use deno_core::futures::StreamExt;
 use deno_core::located_script_name;
 use deno_core::serde_json::json;
-use deno_core::url::Url;
 use deno_core::ModuleSpecifier;
 use deno_runtime::permissions::Permissions;
 use rand::rngs::SmallRng;
@@ -222,48 +219,6 @@ pub(crate) fn is_supported(p: &Path) -> bool {
   }
 }
 
-pub fn is_remote_url(module_url: &str) -> bool {
-  let lower = module_url.to_lowercase();
-  lower.starts_with("http://") || lower.starts_with("https://")
-}
-
-pub fn collect_test_module_specifiers<P>(
-  include: Vec<String>,
-  root_path: &Path,
-  predicate: P,
-) -> Result<Vec<Url>, AnyError>
-where
-  P: Fn(&Path) -> bool,
-{
-  let (include_paths, include_urls): (Vec<String>, Vec<String>) =
-    include.into_iter().partition(|n| !is_remote_url(n));
-  let mut prepared = vec![];
-
-  for path in include_paths {
-    let p = normalize_path(&root_path.join(path));
-    if p.is_dir() {
-      let test_files = collect_files(&[p], &[], &predicate).unwrap();
-      let mut test_files_as_urls = test_files
-        .iter()
-        .map(|f| Url::from_file_path(f).unwrap())
-        .collect::<Vec<Url>>();
-
-      test_files_as_urls.sort();
-      prepared.extend(test_files_as_urls);
-    } else {
-      let url = Url::from_file_path(p).unwrap();
-      prepared.push(url);
-    }
-  }
-
-  for remote_url in include_urls {
-    let url = Url::parse(&remote_url)?;
-    prepared.push(url);
-  }
-
-  Ok(prepared)
-}
-
 pub async fn test_specifier(
   program_state: Arc<ProgramState>,
   main_module: ModuleSpecifier,
@@ -740,37 +695,6 @@ pub async fn run_tests(
 mod tests {
   use super::*;
 
-  #[test]
-  fn test_collect_test_module_specifiers() {
-    let sub_dir_path = test_util::testdata_path().join("subdir");
-    let mut matched_urls = collect_test_module_specifiers(
-      vec![
-        "https://example.com/colors_test.ts".to_string(),
-        "./mod1.ts".to_string(),
-        "./mod3.js".to_string(),
-        "subdir2/mod2.ts".to_string(),
-        "http://example.com/printf_test.ts".to_string(),
-      ],
-      &sub_dir_path,
-      is_supported,
-    )
-    .unwrap();
-    let test_data_url = Url::from_file_path(sub_dir_path).unwrap().to_string();
-
-    let expected: Vec<Url> = vec![
-      format!("{}/mod1.ts", test_data_url),
-      format!("{}/mod3.js", test_data_url),
-      format!("{}/subdir2/mod2.ts", test_data_url),
-      "http://example.com/printf_test.ts".to_string(),
-      "https://example.com/colors_test.ts".to_string(),
-    ]
-    .into_iter()
-    .map(|f| Url::parse(&f).unwrap())
-    .collect();
-    matched_urls.sort();
-    assert_eq!(matched_urls, expected);
-  }
-
   #[test]
   fn test_is_supported() {
     assert!(is_supported(Path::new("tests/subdir/foo_test.ts")));
@@ -790,46 +714,4 @@ mod tests {
     assert!(!is_supported(Path::new("notatest.js")));
     assert!(!is_supported(Path::new("NotAtest.ts")));
   }
-
-  #[test]
-  fn supports_dirs() {
-    // TODO(caspervonb) generate some fixtures in a temporary directory instead, there's no need
-    // for this to rely on external fixtures.
-    let root = test_util::root_path()
-      .join("test_util")
-      .join("std")
-      .join("http");
-    println!("root {:?}", root);
-    let matched_urls = collect_test_module_specifiers(
-      vec![".".to_string()],
-      &root,
-      is_supported,
-    )
-    .unwrap();
-
-    let root_url = Url::from_file_path(root).unwrap().to_string();
-    println!("root_url {}", root_url);
-    let expected: Vec<Url> = vec![
-      format!("{}/_io_test.ts", root_url),
-      format!("{}/cookie_test.ts", root_url),
-      format!("{}/file_server_test.ts", root_url),
-      format!("{}/racing_server_test.ts", root_url),
-      format!("{}/server_test.ts", root_url),
-      format!("{}/test.ts", root_url),
-    ]
-    .into_iter()
-    .map(|f| Url::parse(&f).unwrap())
-    .collect();
-    assert_eq!(matched_urls, expected);
-  }
-
-  #[test]
-  fn test_is_remote_url() {
-    assert!(is_remote_url("https://deno.land/std/http/file_server.ts"));
-    assert!(is_remote_url("http://deno.land/std/http/file_server.ts"));
-    assert!(is_remote_url("HTTP://deno.land/std/http/file_server.ts"));
-    assert!(is_remote_url("HTTp://deno.land/std/http/file_server.ts"));
-    assert!(!is_remote_url("file:///dev/deno_std/http/file_server.ts"));
-    assert!(!is_remote_url("./dev/deno_std/http/file_server.ts"));
-  }
 }