From 556dcff5e318191dd6fbe444c8744ae2b9446c92 Mon Sep 17 00:00:00 2001 From: Bryant Mairs Date: Tue, 7 Feb 2017 00:55:23 -0800 Subject: [PATCH 1/3] Fix intermittent badge::update_crate failures Introduced in 4ce099b. Closes #546. --- src/tests/badge.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tests/badge.rs b/src/tests/badge.rs index d31dc037107..55d659194d7 100644 --- a/src/tests/badge.rs +++ b/src/tests/badge.rs @@ -106,7 +106,10 @@ fn update_crate() { badge_attributes_travis_ci2.clone() ); Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![gitlab.clone(), travis_ci2.clone()]); + let current_badges = krate.badges(tx(&req)).unwrap(); + assert_eq!(current_badges.len(), 2); + assert!(current_badges.contains(&gitlab)); + assert!(current_badges.contains(&travis_ci2)); // Removing one badge badges.clear(); From 765930df63aac634ce94eeb6b8e08346a9bf7ea1 Mon Sep 17 00:00:00 2001 From: Bryant Mairs Date: Tue, 7 Feb 2017 00:51:04 -0800 Subject: [PATCH 2/3] Split up badge tests into smaller ones --- src/tests/badge.rs | 230 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 176 insertions(+), 54 deletions(-) diff --git a/src/tests/badge.rs b/src/tests/badge.rs index 55d659194d7..bef19945075 100644 --- a/src/tests/badge.rs +++ b/src/tests/badge.rs @@ -1,15 +1,25 @@ use conduit::{Request, Method}; +use conduit_test::MockRequest; use postgres::GenericConnection; -use cargo_registry::db::RequestTransaction; use cargo_registry::badge::Badge; +use cargo_registry::db::RequestTransaction; +use cargo_registry::krate::Crate; use std::collections::HashMap; +struct BadgeRef { + appveyor: Badge, + appveyor_attributes: HashMap, + travis_ci: Badge, + travis_ci_attributes: HashMap, + gitlab: Badge, + gitlab_attributes: HashMap, +} + fn tx(req: &Request) -> &GenericConnection { req.tx().unwrap() } -#[test] -fn update_crate() { +fn set_up() -> (MockRequest, Crate, BadgeRef) { let (_b, app, _middle) = ::app(); let mut req = ::req(app, Method::Get, "/api/v1/crates/badged_crate"); @@ -59,39 +69,113 @@ fn update_crate() { String::from("rust-lang/rust") ); - let mut badges = HashMap::new(); + let badges = BadgeRef { + appveyor: appveyor, + appveyor_attributes: badge_attributes_appveyor, + travis_ci: travis_ci, + travis_ci_attributes: badge_attributes_travis_ci, + gitlab: gitlab, + gitlab_attributes: badge_attributes_gitlab, + }; + (req, krate, badges) +} + +#[test] +fn update_no_badges() { + // Add no badges + let (req, krate, _) = set_up(); + + let badges = HashMap::new(); // Updating with no badges has no effect - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); +} + +#[test] +fn update_add_appveyor() { + // Add an appveyor badge + let (req, krate, test_badges) = set_up(); - // Happy path adding one badge + let mut badges = HashMap::new(); badges.insert( String::from("appveyor"), - badge_attributes_appveyor.clone() + test_badges.appveyor_attributes ); - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![appveyor.clone()]); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.appveyor]); +} - // Replacing one badge with another - badges.clear(); +#[test] +fn update_add_travis_ci() { + // Add a travis ci badge + let (req, krate, test_badges) = set_up(); + + let mut badges = HashMap::new(); badges.insert( String::from("travis-ci"), - badge_attributes_travis_ci.clone() + test_badges.travis_ci_attributes ); - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![travis_ci.clone()]); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.travis_ci]); +} - // Replacing one badge with another (again) - badges.clear(); +#[test] +fn update_add_gitlab() { + // Add a gitlab badge + let (req, krate, test_badges) = set_up(); + + let mut badges = HashMap::new(); badges.insert( String::from("gitlab"), - badge_attributes_gitlab.clone() + test_badges.gitlab_attributes + ); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.gitlab]); +} + +#[test] +fn replace_badge() { + // Replacing one badge with another + let (req, krate, test_badges) = set_up(); + + // Add a badge + let mut badges = HashMap::new(); + badges.insert( + String::from("gitlab"), + test_badges.gitlab_attributes ); Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![gitlab.clone()]); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.gitlab]); - // Updating badge attributes + // Replace with another badge + badges.clear(); + badges.insert( + String::from("travis-ci"), + test_badges.travis_ci_attributes.clone() + ); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.travis_ci]); +} + +#[test] +fn update_attributes() { + // Update badge attributes + let (req, krate, test_badges) = set_up(); + + // Add a travis-ci badge + let mut badges = HashMap::new(); + badges.insert( + String::from("travis-ci"), + test_badges.travis_ci_attributes + ); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + let current_badges = krate.badges(tx(&req)).unwrap(); + assert_eq!(current_badges.len(), 1); + assert!(current_badges.contains(&test_badges.travis_ci)); + + // Now update the travis ci badge with different attributes + let mut badges = HashMap::new(); let travis_ci2 = Badge::TravisCi { branch: None, repository: String::from("rust-lang/rust"), @@ -105,73 +189,115 @@ fn update_crate() { String::from("travis-ci"), badge_attributes_travis_ci2.clone() ); - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); let current_badges = krate.badges(tx(&req)).unwrap(); - assert_eq!(current_badges.len(), 2); - assert!(current_badges.contains(&gitlab)); + assert_eq!(current_badges.len(), 1); assert!(current_badges.contains(&travis_ci2)); +} - // Removing one badge - badges.clear(); - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); +#[test] +fn clear_badges() { + // Add 3 badges and then remove them + let (req, krate, test_badges) = set_up(); + + let mut badges = HashMap::new(); // Adding 3 badges badges.insert( String::from("appveyor"), - badge_attributes_appveyor.clone() + test_badges.appveyor_attributes ); badges.insert( String::from("travis-ci"), - badge_attributes_travis_ci.clone() + test_badges.travis_ci_attributes ); badges.insert( String::from("gitlab"), - badge_attributes_gitlab.clone() + test_badges.gitlab_attributes ); - Badge::update_crate( - tx(&req), &krate, badges.clone() - ).unwrap(); + Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); let current_badges = krate.badges(tx(&req)).unwrap(); assert_eq!(current_badges.len(), 3); - assert!(current_badges.contains(&appveyor)); - assert!(current_badges.contains(&travis_ci)); - assert!(current_badges.contains(&gitlab)); + assert!(current_badges.contains(&test_badges.appveyor)); + assert!(current_badges.contains(&test_badges.travis_ci)); + assert!(current_badges.contains(&test_badges.gitlab)); // Removing all badges badges.clear(); - Badge::update_crate(tx(&req), &krate, badges.clone()).unwrap(); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); +} - // Attempting to add one valid badge (appveyor) and three invalid badges - // (travis-ci and gitlab without a required attribute and an unknown badge - // type) +#[test] +fn appveyor_extra_keys() { + // Add a badge with extra invalid keys + let (req, krate, test_badges) = set_up(); - // Extra invalid keys are fine, we'll just ignore those - badge_attributes_appveyor.insert( + let mut badges = HashMap::new(); + + // Extra invalid keys are fine, they just get ignored + let mut appveyor_attributes = test_badges.appveyor_attributes.clone(); + appveyor_attributes.insert( String::from("extra"), String::from("info") ); badges.insert( String::from("appveyor"), - badge_attributes_appveyor.clone() + test_badges.appveyor_attributes ); + Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![test_badges.appveyor]); +} + +#[test] +fn travis_ci_required_keys() { + // Add a travis ci badge missing a required field + let (req, krate, mut test_badges) = set_up(); + + let mut badges = HashMap::new(); + // Repository is a required key - badge_attributes_travis_ci.remove("repository"); + test_badges.travis_ci_attributes.remove("repository"); badges.insert( String::from("travis-ci"), - badge_attributes_travis_ci.clone() + test_badges.travis_ci_attributes ); + let invalid_badges = Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(invalid_badges.len(), 1); + assert!(invalid_badges.contains(&String::from("travis-ci"))); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); +} + +#[test] +fn gitlab_required_keys() { + // Add a gitlab badge missing a required field + let (req, krate, mut test_badges) = set_up(); + + let mut badges = HashMap::new(); + // Repository is a required key - badge_attributes_gitlab.remove("repository"); + test_badges.gitlab_attributes.remove("repository"); badges.insert( String::from("gitlab"), - badge_attributes_gitlab.clone() + test_badges.gitlab_attributes ); + let invalid_badges = Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(invalid_badges.len(), 1); + assert!(invalid_badges.contains(&String::from("gitlab"))); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); +} + +#[test] +fn unknown_badge() { + // Add an unknown badge + let (req, krate, _) = set_up(); + + let mut badges = HashMap::new(); + // This is not a badge that crates.io knows about let mut invalid_attributes = HashMap::new(); invalid_attributes.insert( @@ -180,15 +306,11 @@ fn update_crate() { ); badges.insert( String::from("not-a-badge"), - invalid_attributes.clone() + invalid_attributes ); - let invalid_badges = Badge::update_crate( - tx(&req), &krate, badges.clone() - ).unwrap(); - assert_eq!(invalid_badges.len(), 3); - assert!(invalid_badges.contains(&String::from("travis-ci"))); - assert!(invalid_badges.contains(&String::from("gitlab"))); + let invalid_badges = Badge::update_crate(tx(&req), &krate, badges).unwrap(); + assert_eq!(invalid_badges.len(), 1); assert!(invalid_badges.contains(&String::from("not-a-badge"))); - assert_eq!(krate.badges(tx(&req)).unwrap(), vec![appveyor.clone()]); + assert_eq!(krate.badges(tx(&req)).unwrap(), vec![]); } From 2ecaa880037c160a7a2b4edf92d329fcf4e0f7b4 Mon Sep 17 00:00:00 2001 From: Bryant Mairs Date: Tue, 7 Feb 2017 22:48:55 -0800 Subject: [PATCH 3/3] Correct GitLab capitalization --- app/components/badge-gitlab.js | 2 +- src/badge.rs | 12 ++++++------ src/tests/badge.rs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/components/badge-gitlab.js b/app/components/badge-gitlab.js index 0977a1d6b2f..daa9b1823df 100644 --- a/app/components/badge-gitlab.js +++ b/app/components/badge-gitlab.js @@ -8,6 +8,6 @@ export default Ember.Component.extend({ return this.get('badge.attributes.branch') || 'master'; }), text: Ember.computed('badge', function() { - return `Gitlab build status for the ${ this.get('branch') } branch`; + return `GitLab build status for the ${ this.get('branch') } branch`; }) }); diff --git a/src/badge.rs b/src/badge.rs index 3d12321b0ad..30dea5a8bee 100644 --- a/src/badge.rs +++ b/src/badge.rs @@ -15,7 +15,7 @@ pub enum Badge { Appveyor { repository: String, branch: Option, service: Option, }, - Gitlab { + GitLab { repository: String, branch: Option, }, } @@ -62,14 +62,14 @@ impl Model for Badge { } }, "gitlab" => { - Badge::Gitlab { + Badge::GitLab { branch: attributes.get("branch") .and_then(Json::as_string) .map(str::to_string), repository: attributes.get("repository") .and_then(Json::as_string) .map(str::to_string) - .expect("Invalid Gitlab badge \ + .expect("Invalid GitLab badge \ without repository in the \ database"), } @@ -100,7 +100,7 @@ impl Badge { match *self { Badge::TravisCi {..} => "travis-ci", Badge::Appveyor {..} => "appveyor", - Badge::Gitlab{..} => "gitlab", + Badge::GitLab{..} => "gitlab", } } @@ -138,7 +138,7 @@ impl Badge { ); } }, - Badge::Gitlab { branch, repository } => { + Badge::GitLab { branch, repository } => { attributes.insert(String::from("repository"), repository); if let Some(branch) = branch { attributes.insert( @@ -186,7 +186,7 @@ impl Badge { "gitlab" => { match attributes.get("repository") { Some(repository) => { - Ok(Badge::Gitlab { + Ok(Badge::GitLab { repository: repository.to_string(), branch: attributes.get("branch") .map(String::to_string), diff --git a/src/tests/badge.rs b/src/tests/badge.rs index bef19945075..6715590cee4 100644 --- a/src/tests/badge.rs +++ b/src/tests/badge.rs @@ -55,7 +55,7 @@ fn set_up() -> (MockRequest, Crate, BadgeRef) { String::from("rust-lang/rust") ); - let gitlab = Badge::Gitlab { + let gitlab = Badge::GitLab { branch: Some(String::from("beta")), repository: String::from("rust-lang/rust"), };