fix tests and remove unwraps

Author: Heliozoa

tmc-langs-util/src/error.rs

@@ -32,6 +32,8 @@ pub enum UtilError {
     #[error("Error while writing file to zip")]
     ZipWrite(#[source] std::io::Error),
 
+    #[error("Cache path {0} was invalid. Not a valid UTF-8 string or did not contain a cache version after a dash")]
+    InvalidCachePath(PathBuf),
     #[error("Path {0} contained a dash '-' which is currently not allowed")]
     InvalidDirectory(PathBuf),
 
@@ -43,6 +45,8 @@ pub enum UtilError {
     Zip(#[from] zip::result::ZipError),
     #[error(transparent)]
     FileIo(#[from] FileIo),
+    #[error(transparent)]
+    Yaml(#[from] serde_yaml::Error),
 
     #[cfg(unix)]
     #[error("Error changing permissions of {0}")]

tmc-langs-util/src/task_executor.rs

@@ -207,14 +207,18 @@ pub fn clean(path: &Path) -> Result<(), UtilError> {
 
 /// Recursively searches for valid exercise directories in the path.
 pub fn find_exercise_directories(exercise_path: &Path) -> Result<Vec<PathBuf>, UtilError> {
+    log::info!(
+        "finding exercise directories in {}",
+        exercise_path.display()
+    );
+
     let mut paths = vec![];
     for entry in WalkDir::new(exercise_path).into_iter().filter_entry(|e| {
         !submission_processing::is_hidden_dir(e)
             && e.file_name() != "private"
             && !submission_processing::contains_tmcignore(e)
     }) {
         let entry = entry?;
-        // TODO: Java implementation doesn't scan root directories
         if is_exercise_root_directory(entry.path()) {
             paths.push(entry.into_path())
         }

tmc-langs-util/src/task_executor/course_refresher.rs

@@ -68,13 +68,12 @@ impl CourseRefresher {
         // create new cache path
         let old_version = course_cache_path
             .to_str()
-            .unwrap()
-            .split('-')
-            .last()
-            .unwrap()
-            .parse::<u32>()
-            .unwrap();
+            .and_then(|s| s.split('-').last())
+            .and_then(|s| s.parse::<u32>().ok())
+            .ok_or_else(|| UtilError::InvalidCachePath(course_cache_path.clone()))?;
         let new_cache_path = cache_root.join(format!("{}-{}", course_name, old_version + 1));
+        log::info!("next cache path: {}", new_cache_path.display());
+
         if new_cache_path.exists() {
             log::info!("clearing new cache path at {}", new_cache_path.display());
             file_util::remove_dir_all(&new_cache_path)?;
@@ -85,9 +84,8 @@ impl CourseRefresher {
 
         // initialize new clone path and verify directory names
         let new_clone_path = new_cache_path.join("clone");
-        log::info!("updating repository to {}", new_clone_path.display());
         let old_clone_path = course_cache_path.join("clone");
-        update_or_clone_repository(
+        initialize_new_cache_clone(
             &new_cache_path,
             &new_clone_path,
             &old_clone_path,
@@ -98,7 +96,6 @@ impl CourseRefresher {
         self.progress_reporter
             .finish_step("Updated repository".to_string(), None)?;
 
-        log::info!("updating course options");
         let course_options = get_course_options(&new_clone_path, &course_name)?;
         self.progress_reporter
             .finish_step("Updated course options".to_string(), None)?;
@@ -108,7 +105,7 @@ impl CourseRefresher {
 
         let exercise_dirs = task_executor::find_exercise_directories(&new_clone_path)?
             .into_iter()
-            .map(|ed| ed.strip_prefix(&new_clone_path).unwrap().to_path_buf())
+            .map(|ed| ed.strip_prefix(&new_clone_path).unwrap().to_path_buf()) // safe
             .collect();
 
         // make_solutions
@@ -133,23 +130,23 @@ impl CourseRefresher {
         self.progress_reporter
             .finish_step("Prepared stubs".to_string(), None)?;
 
-        // find exercises in new clone path
-        log::info!("finding exercises");
-        // (exercise name, exercise path)
         let exercises = get_exercises(exercise_dirs, &new_clone_path, &new_stub_path)?;
         self.progress_reporter
             .finish_step("Located exercises".to_string(), None)?;
 
         // make_zips_of_solutions
         let new_solution_zip_path = new_cache_path.join("solution_zip");
-        log::info!("compressing solutions");
         execute_zip(&exercises, &new_solution_path, &new_solution_zip_path)?;
         self.progress_reporter
             .finish_step("Compressed solutions".to_string(), None)?;
 
         // make_zips_of_stubs
         let new_stub_zip_path = new_cache_path.join("stub_zip");
-        log::info!("compressing stubs");
+        log::info!(
+            "compressing stubs from {} to {}",
+            new_stub_path.display(),
+            new_stub_zip_path.display()
+        );
         execute_zip(&exercises, &new_stub_path, &new_stub_zip_path)?;
         self.progress_reporter
             .finish_step("Compressed stubs".to_string(), None)?;
@@ -193,15 +190,17 @@ pub fn refresh_course(
 /// If found, copies it to course_clone_path fetches origin from course_source_url, checks out origin/course_git_branch, cleans and checks out the repo.
 /// If not found or found but one of the git commands causes an error, deletes course_clone_path and clones course_git_branch from course_source_url there.
 /// NOP during testing.
-fn update_or_clone_repository(
+fn initialize_new_cache_clone(
     new_course_root: &Path,
     new_clone_path: &Path,
     old_clone_path: &Path,
     course_source_url: &str,
     course_git_branch: &str,
 ) -> Result<(), UtilError> {
+    log::info!("initializing repository at {}", new_clone_path.display());
+
     if old_clone_path.join(".git").exists() {
-        // Try a fast path: copy old clone and git fetch new stuff
+        log::info!("trying to copy clone from previous cache");
 
         // closure to collect any error that occurs during the process
         let copy_and_update_repository = || -> Result<(), UtilError> {
@@ -238,6 +237,8 @@ fn update_or_clone_repository(
         }
     };
 
+    log::info!("could not copy from previous cache, cloning");
+
     // clone_repository
     TmcCommand::new("git".to_string())
         .with(|e| {
@@ -255,9 +256,11 @@ fn update_or_clone_repository(
 /// Makes sure no directory directly under path is an exercise directory containing a dash in the relative path from path to the dir.
 /// A dash is used as a special delimiter.
 fn check_directory_names(path: &Path) -> Result<(), UtilError> {
+    log::info!("checking directory names for dashes");
+
     // exercise directories in canonicalized form
     for exercise_dir in task_executor::find_exercise_directories(path)? {
-        let relative = exercise_dir.strip_prefix(path).unwrap();
+        let relative = exercise_dir.strip_prefix(path).unwrap(); // safe
         if relative.to_string_lossy().contains('-') {
             return Err(UtilError::InvalidDirectory(exercise_dir));
         }
@@ -269,10 +272,16 @@ fn check_directory_names(path: &Path) -> Result<(), UtilError> {
 /// If found, course-specific options are merged into it and it is returned.
 /// Else, an empty mapping is returned.
 fn get_course_options(course_clone_path: &Path, course_name: &str) -> Result<Mapping, UtilError> {
+    log::info!(
+        "collecting course options for {} in {}",
+        course_name,
+        course_clone_path.display()
+    );
+
     let options_file = course_clone_path.join("course_options.yml");
     if options_file.exists() {
         let file = file_util::open_file(options_file)?;
-        let mut course_options: Mapping = serde_yaml::from_reader(file).unwrap();
+        let mut course_options: Mapping = serde_yaml::from_reader(file)?;
         // try to remove the "courses" map
         if let Some(serde_yaml::Value::Mapping(mut courses)) =
             course_options.remove(&serde_yaml::Value::String("courses".to_string()))
@@ -300,6 +309,8 @@ fn get_exercises(
     course_clone_path: &Path,
     course_stub_path: &Path,
 ) -> Result<Vec<RefreshExercise>, UtilError> {
+    log::info!("finding exercise checksums and points");
+
     let exercises = exercise_dirs
         .into_iter()
         .map(|exercise_dir| {
@@ -308,10 +319,7 @@ fn get_exercises(
                 exercise_dir.display()
             );
             let name = exercise_dir.to_string_lossy().replace("/", "-");
-
-            // checksum
             let checksum = calculate_checksum(&course_stub_path.join(&exercise_dir))?;
-
             let exercise_path = course_clone_path.join(&exercise_dir);
             let points = task_executor::get_available_points(&exercise_path)?;
 
@@ -335,12 +343,10 @@ fn calculate_checksum(exercise_dir: &Path) -> Result<String, UtilError> {
         .sort_by(|a, b| a.file_name().cmp(b.file_name()))
     {
         let entry = entry?;
-        let relative = entry.path().strip_prefix(exercise_dir).unwrap();
+        let relative = entry.path().strip_prefix(exercise_dir).unwrap(); // safe
         let string = relative.as_os_str().to_string_lossy();
-        log::debug!("updating {}", string);
         digest.consume(string.as_ref());
         if entry.path().is_file() {
-            log::debug!("updating with file");
             let file = file_util::read_file(entry.path())?;
             digest.consume(file);
         }
@@ -356,6 +362,12 @@ fn execute_zip(
     root_path: &Path,
     zip_dir: &Path,
 ) -> Result<(), UtilError> {
+    log::info!(
+        "compressing exercises from from {} to {}",
+        root_path.display(),
+        zip_dir.display()
+    );
+
     file_util::create_dir_all(zip_dir)?;
     for exercise in course_exercises {
         let exercise_root = root_path.join(&exercise.path);
@@ -373,16 +385,15 @@ fn execute_zip(
             let entry = entry?;
             if entry.path().is_file() {
                 let relative_path = entry.path().strip_prefix(&root_path).unwrap(); // safe
-                writer
-                    .start_file(
-                        relative_path.to_string_lossy(),
-                        zip::write::FileOptions::default(),
-                    )
-                    .unwrap();
+                writer.start_file(
+                    relative_path.to_string_lossy(),
+                    zip::write::FileOptions::default(),
+                )?;
                 let bytes = file_util::read_file(entry.path())?;
                 writer.write_all(&bytes).map_err(UtilError::ZipWrite)?;
             }
         }
+        writer.finish()?;
     }
     Ok(())
 }
@@ -398,6 +409,8 @@ fn set_permissions(path: &Path) -> Result<(), UtilError> {
     use nix::sys::stat;
     use std::os::unix::io::AsRawFd;
 
+    log::info!("setting permissions in {}", path.display());
+
     let chmod: ModeBits = 0o775; // octal, read and execute permissions for all users
     for entry in WalkDir::new(path) {
         let entry = entry?;
@@ -555,7 +568,15 @@ courses:
             "course/part1/ex1/test/test.py",
             "@points('1') @points('2')",
         );
-        let exercise_dirs = find_exercise_directories(&temp.path().join("course")).unwrap();
+        let exercise_dirs = find_exercise_directories(&temp.path().join("course"))
+            .unwrap()
+            .into_iter()
+            .map(|ed| {
+                ed.strip_prefix(&temp.path().join("course"))
+                    .unwrap()
+                    .to_path_buf()
+            })
+            .collect();
         let exercises = get_exercises(
             exercise_dirs,
             &temp.path().join("course"),
@@ -567,7 +588,7 @@ courses:
         assert_eq!(exercises[0].points.len(), 2);
         assert_eq!(exercises[0].points[0], "1");
         assert_eq!(exercises[0].points[1], "2");
-        assert_eq!(exercises[0].checksum, "043fb4832da4e3fbf5babd13ed9fa732");
+        assert_eq!(exercises[0].checksum, "129e7e898698465c4f24494219f06df9");
     }
 
     #[test]
@@ -588,7 +609,15 @@ courses:
         file_to(&temp, "stub/part2/ex2/dir/subdir/file", "some file");
         file_to(&temp, "stub/part2/ex2/dir/subdir/.hidden", "hidden file");
 
-        let exercise_dirs = find_exercise_directories(&temp.path().join("clone")).unwrap();
+        let exercise_dirs = find_exercise_directories(&temp.path().join("clone"))
+            .unwrap()
+            .into_iter()
+            .map(|ed| {
+                ed.strip_prefix(&temp.path().join("clone"))
+                    .unwrap()
+                    .to_path_buf()
+            })
+            .collect();
         let exercises = get_exercises(
             exercise_dirs,
             &temp.path().join("clone"),
@@ -611,23 +640,26 @@ courses:
         for i in fz.file_names() {
             log::debug!("{}", i);
         }
-        assert!(fz.by_name("setup.py").is_ok());
         assert!(fz
             .by_name(
-                &Path::new("dir")
+                &Path::new("part2")
+                    .join("ex2")
+                    .join("dir")
                     .join("subdir")
                     .join(".hidden")
                     .to_string_lossy()
             )
-            .is_err());
+            .is_err()); // hidden files filtered
         let mut file = fz
             .by_name(
-                &Path::new("dir")
+                &Path::new("part2")
+                    .join("ex2")
+                    .join("dir")
                     .join("subdir")
                     .join("file")
                     .to_string_lossy(),
             )
-            .unwrap();
+            .unwrap(); // other files have their stub contents
         let mut buf = String::new();
         file.read_to_string(&mut buf).unwrap();
         assert_eq!(buf, "some file");

tmc-langs-util/src/task_executor/submission_processing.rs

@@ -159,6 +159,12 @@ fn process_files(
 ///
 /// Binary files are copied without extra processing, while text files are parsed to remove solution tags and stubs.
 pub fn prepare_solution(exercise_path: &Path, dest_root: &Path) -> Result<(), TmcError> {
+    log::debug!(
+        "preparing solution from {} to {}",
+        exercise_path.display(),
+        dest_root.display()
+    );
+
     let line_filter = |meta: &MetaString| !matches!(meta, MetaString::Stub(_));
     let file_filter = |_metas: &[_]| true; // include all files in solution
     process_files(exercise_path, dest_root, line_filter, file_filter)?;
@@ -174,6 +180,12 @@ pub fn prepare_solution(exercise_path: &Path, dest_root: &Path) -> Result<(), Tm
 ///
 /// Additionally, copies any shared files with the corresponding language plugins.
 pub fn prepare_stub(exercise_path: &Path, dest_root: &Path) -> Result<(), TmcError> {
+    log::debug!(
+        "preparing stub from {} to {}",
+        exercise_path.display(),
+        dest_root.display()
+    );
+
     let line_filter = |meta: &MetaString| !matches!(meta, MetaString::Solution(_));
     let file_filter = |metas: &[MetaString]| {
         !metas