From 568b9eb9f78e73bbc76b18528b095ce6c0d8a369 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Thu, 9 Jan 2020 23:24:35 +0100 Subject: [PATCH 1/6] expand create_release test wrapper --- src/web/crate_details.rs | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 1d91820e4..01b2c2744 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -313,20 +313,14 @@ pub fn crate_details_handler(req: &mut Request) -> IronResult { } } + + #[cfg(test)] mod tests { use super::*; use crate::test::TestDatabase; use failure::Error; - fn create_release(db: &TestDatabase, package: &str, version: &str, successful: bool) -> Result { - db.fake_release() - .name(package) - .version(version) - .build_result_successful(successful) - .create() - } - fn assert_last_successful_build_equals( db: &TestDatabase, package: &str, @@ -349,9 +343,9 @@ mod tests { crate::test::wrapper(|env| { let db = env.db(); - create_release(&db, "foo", "0.0.1", true)?; - create_release(&db, "foo", "0.0.2", true)?; - create_release(&db, "foo", "0.0.3", false)?; + db.fake_release().name("foo").version("0.0.1").create()?; + db.fake_release().name("foo").version("0.0.2").create()?; + db.fake_release().name("foo").version("0.0.3").build_result_successful(false).create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; @@ -365,8 +359,8 @@ mod tests { crate::test::wrapper(|env| { let db = env.db(); - create_release(&db, "foo", "0.0.1", false)?; - create_release(&db, "foo", "0.0.2", false)?; + db.fake_release().name("foo").version("0.0.1").build_result_successful(false).create()?; + db.fake_release().name("foo").version("0.0.2").build_result_successful(false).create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; @@ -379,9 +373,9 @@ mod tests { crate::test::wrapper(|env| { let db = env.db(); - create_release(&db, "foo", "0.0.1", true)?; - create_release(&db, "foo", "0.0.2", false)?; - create_release(&db, "foo", "0.0.3", true)?; + db.fake_release().name("foo").version("0.0.1").create()?; + db.fake_release().name("foo").version("0.0.2").build_result_successful(false).create()?; + db.fake_release().name("foo").version("0.0.3").create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", Some("0.0.3"))?; @@ -396,13 +390,13 @@ mod tests { let db = env.db(); // Add new releases of 'foo' out-of-order since CrateDetails should sort them descending - create_release(&db, "foo", "0.1.0", true)?; - create_release(&db, "foo", "0.1.1", true)?; - create_release(&db, "foo", "0.3.0", false)?; - create_release(&db, "foo", "1.0.0", true)?; - create_release(&db, "foo", "0.12.0", true)?; - create_release(&db, "foo", "0.2.0", true)?; - create_release(&db, "foo", "0.2.0-alpha", true)?; + db.fake_release().name("foo").version("0.1.0").create()?; + db.fake_release().name("foo").version("0.1.1").create()?; + db.fake_release().name("foo").version("0.3.0").build_result_successful(false).create()?; + db.fake_release().name("foo").version("1.0.0").create()?; + db.fake_release().name("foo").version("0.12.0").create()?; + db.fake_release().name("foo").version("0.2.0").create()?; + db.fake_release().name("foo").version("0.2.0-alpha").create()?; let details = CrateDetails::new(&db.conn(), "foo", "0.2.0").unwrap(); assert_eq!(details.releases, vec![ From 247d1e3470a19d9330d3eb5b0df11439ee57aa86 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Sat, 11 Jan 2020 00:03:50 +0100 Subject: [PATCH 2/6] last_successful_build: extend tests with yanked crates --- src/test/fakes.rs | 5 +++++ src/web/crate_details.rs | 18 ++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/test/fakes.rs b/src/test/fakes.rs index 2dd7e8504..ac3ed6925 100644 --- a/src/test/fakes.rs +++ b/src/test/fakes.rs @@ -76,6 +76,11 @@ impl<'db> FakeRelease<'db> { self } + pub(crate) fn cratesio_data_yanked(mut self, new: bool) -> Self { + self.cratesio_data.yanked = new; + self + } + pub(crate) fn create(self) -> Result { let tempdir = tempdir::TempDir::new("docs.rs-fake")?; diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 01b2c2744..cd38c9e4e 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -339,47 +339,53 @@ mod tests { } #[test] - fn test_last_successful_build_when_last_release_failed() { + fn test_last_successful_build_when_last_releases_failed_or_yanked() { crate::test::wrapper(|env| { let db = env.db(); db.fake_release().name("foo").version("0.0.1").create()?; db.fake_release().name("foo").version("0.0.2").create()?; db.fake_release().name("foo").version("0.0.3").build_result_successful(false).create()?; + db.fake_release().name("foo").version("0.0.4").cratesio_data_yanked(true).create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.3", Some("0.0.2"))?; + // don't test for foo-0.0.4, yanked crates are not displayed Ok(()) }); } #[test] - fn test_last_successful_build_when_all_releases_failed() { + fn test_last_successful_build_when_all_releases_failed_or_yanked() { crate::test::wrapper(|env| { let db = env.db(); db.fake_release().name("foo").version("0.0.1").build_result_successful(false).create()?; db.fake_release().name("foo").version("0.0.2").build_result_successful(false).create()?; + db.fake_release().name("foo").version("0.0.3").cratesio_data_yanked(true).create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; + // don't test for foo-0.0.3, yanked crates are not displayed Ok(()) }); } #[test] - fn test_last_successful_build_when_an_intermittent_release_failed() { + fn test_last_successful_build_with_intermittent_releases_failed_or_yanked() { crate::test::wrapper(|env| { let db = env.db(); db.fake_release().name("foo").version("0.0.1").create()?; db.fake_release().name("foo").version("0.0.2").build_result_successful(false).create()?; - db.fake_release().name("foo").version("0.0.3").create()?; + db.fake_release().name("foo").version("0.0.3").cratesio_data_yanked(true).create()?; + db.fake_release().name("foo").version("0.0.4").create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; - assert_last_successful_build_equals(&db, "foo", "0.0.2", Some("0.0.3"))?; - assert_last_successful_build_equals(&db, "foo", "0.0.3", None)?; + assert_last_successful_build_equals(&db, "foo", "0.0.2", Some("0.0.4"))?; + // don't test for foo-0.0.3, yanked crates are not displayed + assert_last_successful_build_equals(&db, "foo", "0.0.4", None)?; Ok(()) }); } From 416fd7b95815265d1f31b1a2c18f11fe96beb743 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Sat, 11 Jan 2020 00:20:54 +0100 Subject: [PATCH 3/6] refactor CrateDetails: add yanked to Release This avoids doing an additional query to determine the last successful build. --- src/web/crate_details.rs | 45 ++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index cd38c9e4e..1b11b90f4 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -96,6 +96,7 @@ impl ToJson for CrateDetails { struct Release { pub version: String, pub build_status: bool, + pub yanked: bool, } @@ -235,21 +236,11 @@ impl CrateDetails { crate_details.owners.push((row.get(0), row.get(1))); } - // retrieve last successful build if build failed if !crate_details.build_status { - let rows = conn.query( - "SELECT version - FROM releases - INNER JOIN crates ON releases.crate_id = crates.id - WHERE build_status = true AND yanked = false AND crates.name = $1 - ORDER BY release_time desc - LIMIT 1;", - &[&name], - ).unwrap(); - - if rows.len() >= 1 { - crate_details.last_successful_build = Some(rows.get(0).get(0)); - } + crate_details.last_successful_build = crate_details.releases.iter() + .filter(|release| release.build_status && !release.yanked) + .map(|release| release.version.to_owned()) + .nth(0); } Some(crate_details) @@ -264,19 +255,19 @@ impl CrateDetails { fn map_to_release(conn: &Connection, crate_id: i32, version: String) -> Release { let rows = conn.query( - "SELECT build_status + "SELECT build_status, yanked FROM releases WHERE releases.crate_id = $1 and releases.version = $2;", &[&crate_id, &version], ).unwrap(); - let build_status = if !rows.is_empty() { - rows.get(0).get(0) + let (build_status, yanked) = if !rows.is_empty() { + (rows.get(0).get(0), rows.get(0).get(1)) } else { - false + Default::default() }; - Release { version, build_status } + Release { version, build_status, yanked } } @@ -401,18 +392,18 @@ mod tests { db.fake_release().name("foo").version("0.3.0").build_result_successful(false).create()?; db.fake_release().name("foo").version("1.0.0").create()?; db.fake_release().name("foo").version("0.12.0").create()?; - db.fake_release().name("foo").version("0.2.0").create()?; + db.fake_release().name("foo").version("0.2.0").cratesio_data_yanked(true).create()?; db.fake_release().name("foo").version("0.2.0-alpha").create()?; let details = CrateDetails::new(&db.conn(), "foo", "0.2.0").unwrap(); assert_eq!(details.releases, vec![ - Release { version: "1.0.0".to_string(), build_status: true }, - Release { version: "0.12.0".to_string(), build_status: true }, - Release { version: "0.3.0".to_string(), build_status: false }, - Release { version: "0.2.0".to_string(), build_status: true }, - Release { version: "0.2.0-alpha".to_string(), build_status: true }, - Release { version: "0.1.1".to_string(), build_status: true }, - Release { version: "0.1.0".to_string(), build_status: true }, + Release { version: "1.0.0".to_string(), build_status: true, yanked: false }, + Release { version: "0.12.0".to_string(), build_status: true, yanked: false }, + Release { version: "0.3.0".to_string(), build_status: false, yanked: false }, + Release { version: "0.2.0".to_string(), build_status: true, yanked: true }, + Release { version: "0.2.0-alpha".to_string(), build_status: true, yanked: false }, + Release { version: "0.1.1".to_string(), build_status: true, yanked: false }, + Release { version: "0.1.0".to_string(), build_status: true, yanked: false }, ]); Ok(()) From fa8e926308061916ee42f0f9737bc0d0511fccb8 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Sat, 11 Jan 2020 00:53:31 +0100 Subject: [PATCH 4/6] fields of crate_details::Release don't have to be public --- src/web/crate_details.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 1b11b90f4..727ad453e 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -94,9 +94,9 @@ impl ToJson for CrateDetails { #[derive(Debug, Eq, PartialEq)] struct Release { - pub version: String, - pub build_status: bool, - pub yanked: bool, + version: String, + build_status: bool, + yanked: bool, } From d54aca740a55d7e3bac85ee6d3ce79e71758c0d0 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Sat, 11 Jan 2020 01:40:34 +0100 Subject: [PATCH 5/6] use .next() instead of .nth(0) --- src/web/crate_details.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 727ad453e..c8342a9c8 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -240,7 +240,7 @@ impl CrateDetails { crate_details.last_successful_build = crate_details.releases.iter() .filter(|release| release.build_status && !release.yanked) .map(|release| release.version.to_owned()) - .nth(0); + .next(); } Some(crate_details) From d87822bd7e0c0d74ee3d5aa72a90631c56656d79 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Sat, 11 Jan 2020 02:09:36 +0100 Subject: [PATCH 6/6] update CrateDetails tests to handle yanked crates correctly When a crate is yanked from crates.io, its documentation might not yet be removed from docs.rs. So it is possible to create CrateDetails of a release that was build successfully but yanked afterwards. Since this release was built successfully, it will not have a reference to a last successful build. But since it was yanked, other release will not point to this release as last successful build. --- src/web/crate_details.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index c8342a9c8..e691b5254 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -338,11 +338,13 @@ mod tests { db.fake_release().name("foo").version("0.0.2").create()?; db.fake_release().name("foo").version("0.0.3").build_result_successful(false).create()?; db.fake_release().name("foo").version("0.0.4").cratesio_data_yanked(true).create()?; + db.fake_release().name("foo").version("0.0.5").build_result_successful(false).cratesio_data_yanked(true).create()?; assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.3", Some("0.0.2"))?; - // don't test for foo-0.0.4, yanked crates are not displayed + assert_last_successful_build_equals(&db, "foo", "0.0.4", None)?; + assert_last_successful_build_equals(&db, "foo", "0.0.5", Some("0.0.2"))?; Ok(()) }); } @@ -358,7 +360,7 @@ mod tests { assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", None)?; - // don't test for foo-0.0.3, yanked crates are not displayed + assert_last_successful_build_equals(&db, "foo", "0.0.3", None)?; Ok(()) }); } @@ -375,7 +377,7 @@ mod tests { assert_last_successful_build_equals(&db, "foo", "0.0.1", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.2", Some("0.0.4"))?; - // don't test for foo-0.0.3, yanked crates are not displayed + assert_last_successful_build_equals(&db, "foo", "0.0.3", None)?; assert_last_successful_build_equals(&db, "foo", "0.0.4", None)?; Ok(()) });