Skip to content

tests: Use more doc comments to describe tests and test helpers #5540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 26, 2022
Merged

tests: Use more doc comments to describe tests and test helpers #5540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 27 additions & 31 deletions src/tests/owners.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,8 @@ fn create_and_add_owner(
user
}

// Ensures that so long as at least one owner remains associated with the crate,
// a user can still remove their own login as an owner
/// Ensures that so long as at least one owner remains associated with the crate,
/// a user can still remove their own login as an owner
#[test]
fn owners_can_remove_self() {
let (app, _, user, token) = TestApp::init().with_token();
Expand Down Expand Up @@ -207,7 +207,7 @@ fn owners_can_remove_self() {
);
}

// Verify consistency when adidng or removing multiple owners in a single request.
/// Verify consistency when adidng or removing multiple owners in a single request.
#[test]
fn modify_multiple_owners() {
let (app, _, user, token) = TestApp::init().with_token();
Expand Down Expand Up @@ -406,13 +406,12 @@ fn invite_with_existing_expired_invite() {
assert_eq!(2, app.as_inner().emails.mails_in_memory().unwrap().len());
}

/* Testing the crate ownership between two crates and one team.
Given two crates, one crate owned by both a team and a user,
one only owned by a user, check that the CrateList returned
for the user_id contains only the crates owned by that user,
and that the CrateList returned for the team_id contains
only crates owned by that team.
*/
/// Testing the crate ownership between two crates and one team.
/// Given two crates, one crate owned by both a team and a user,
/// one only owned by a user, check that the CrateList returned
/// for the user_id contains only the crates owned by that user,
/// and that the CrateList returned for the team_id contains
/// only crates owned by that team.
#[test]
fn check_ownership_two_crates() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down Expand Up @@ -440,14 +439,13 @@ fn check_ownership_two_crates() {
assert_eq!(json.crates[0].name, krate_owned_by_team.name);
}

/* Given a crate owned by both a team and a user, check that the
JSON returned by the /owner_team route and /owner_user route
contains the correct kind of owner

Note that in this case function new_team must take a team name
of form github:org_name:team_name as that is the format
EncodableOwner::encodable is expecting
*/
/// Given a crate owned by both a team and a user, check that the
/// JSON returned by the /owner_team route and /owner_user route
/// contains the correct kind of owner
///
/// Note that in this case function new_team must take a team name
/// of form github:org_name:team_name as that is the format
/// EncodableOwner::encodable is expecting
#[test]
fn check_ownership_one_crate() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down Expand Up @@ -572,12 +570,11 @@ fn invitations_list_does_not_include_expired_invites_v1() {
);
}

/* Given a user inviting a different user to be a crate
owner, check that the user invited can accept their
invitation, the invitation will be deleted from
the invitations table, and a new crate owner will be
inserted into the table for the given crate.
*/
/// Given a user inviting a different user to be a crate
/// owner, check that the user invited can accept their
/// invitation, the invitation will be deleted from
/// the invitations table, and a new crate owner will be
/// inserted into the table for the given crate.
#[test]
fn test_accept_invitation() {
let (app, anon, owner, owner_token) = TestApp::init().with_token();
Expand All @@ -600,11 +597,10 @@ fn test_accept_invitation() {
assert_eq!(json.users.len(), 2);
}

/* Given a user inviting a different user to be a crate
owner, check that the user invited can decline their
invitation and the invitation will be deleted from
the invitations table.
*/
/// Given a user inviting a different user to be a crate
/// owner, check that the user invited can decline their
/// invitation and the invitation will be deleted from
/// the invitations table.
#[test]
fn test_decline_invitation() {
let (app, anon, owner, owner_token) = TestApp::init().with_token();
Expand Down Expand Up @@ -652,8 +648,8 @@ fn test_accept_invitation_by_mail() {
assert_eq!(json.users.len(), 2);
}

/// Hacky way to simulate the expiration of an ownership invitation. Instead of letting a month
/// pass, the creation date of the invite is moved back a month.
//// Hacky way to simulate the expiration of an ownership invitation. Instead of letting a month
//// pass, the creation date of the invite is moved back a month.
fn expire_invitation(app: &TestApp, crate_id: i32) {
use cargo_registry::schema::crate_owner_invitations;

Expand Down
4 changes: 2 additions & 2 deletions src/tests/record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ static IGNORED_HEADERS: &[&str] = &[
"user-agent",
];

// A "bomb" so when the test task exists we know when to shut down
// the server and fail if the subtask failed.
/// A "bomb" so when the test task exists we know when to shut down
/// the server and fail if the subtask failed.
pub struct Bomb {
iorx: Sink,
quittx: Option<oneshot::Sender<()>>,
Expand Down
27 changes: 12 additions & 15 deletions src/tests/routes/crates/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,13 +554,12 @@ fn max_stable_version() {
assert_eq!(json.crates[0].max_stable_version, Some("1.0.0".to_string()));
}

/* Given two crates, one with downloads less than 90 days ago, the
other with all downloads greater than 90 days ago, check that
the order returned is by recent downloads, descending. Check
also that recent download counts are returned in recent_downloads,
and total downloads counts are returned in downloads, and that
these numbers do not overlap.
*/
/// Given two crates, one with downloads less than 90 days ago, the
/// other with all downloads greater than 90 days ago, check that
/// the order returned is by recent downloads, descending. Check
/// also that recent download counts are returned in recent_downloads,
/// and total downloads counts are returned in downloads, and that
/// these numbers do not overlap.
#[test]
fn test_recent_download_count() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down Expand Up @@ -595,10 +594,9 @@ fn test_recent_download_count() {
assert_eq!(json.crates[1].downloads, 10);
}

/* Given one crate with zero downloads, check that the crate
still shows up in index results, but that it displays 0
for both recent downloads and downloads.
*/
/// Given one crate with zero downloads, check that the crate
/// still shows up in index results, but that it displays 0
/// for both recent downloads and downloads.
#[test]
fn test_zero_downloads() {
let (app, anon, user) = TestApp::init().with_user();
Expand All @@ -620,10 +618,9 @@ fn test_zero_downloads() {
assert_eq!(json.crates[0].downloads, 0);
}

/* Given two crates, one with more all-time downloads, the other with
more downloads in the past 90 days, check that the index page for
categories and keywords is sorted by recent downloads by default.
*/
/// Given two crates, one with more all-time downloads, the other with
/// more downloads in the past 90 days, check that the index page for
/// categories and keywords is sorted by recent downloads by default.
#[test]
fn test_default_sort_recent() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down
16 changes: 8 additions & 8 deletions src/tests/team.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ impl crate::util::MockAnonymousUser {
}
}

// Test adding team without `github:`
/// Test adding team without `github:`
#[test]
fn not_github() {
let (app, _, user, token) = TestApp::init().with_token();
Expand Down Expand Up @@ -49,7 +49,7 @@ fn weird_name() {
);
}

// Test adding team without second `:`
/// Test adding team without second `:`
#[test]
fn one_colon() {
let (app, _, user, token) = TestApp::init().with_token();
Expand Down Expand Up @@ -82,7 +82,7 @@ fn nonexistent_team() {
);
}

// Test adding a renamed team
/// Test adding a renamed team
#[test]
fn add_renamed_team() {
let (app, anon) = TestApp::init().empty();
Expand Down Expand Up @@ -119,7 +119,7 @@ fn add_renamed_team() {
assert_eq!(json.teams[0].login, "github:test-org:core");
}

// Test adding team names with mixed case, when on the team
/// Test adding team names with mixed case, when on the team
#[test]
fn add_team_mixed_case() {
let (app, anon) = TestApp::init().empty();
Expand Down Expand Up @@ -174,7 +174,7 @@ fn add_team_as_org_owner() {
assert_eq!(json.teams[0].login, "github:test-org:core");
}

// Test adding team as owner when not on it
/// Test adding team as owner when not on it
#[test]
fn add_team_as_non_member() {
let (app, _) = TestApp::init().empty();
Expand Down Expand Up @@ -268,7 +268,7 @@ fn remove_team_as_team_owner() {
);
}

// Test trying to publish a crate we don't own
/// Test trying to publish a crate we don't own
#[test]
fn publish_not_owned() {
let (app, _) = TestApp::full().empty();
Expand Down Expand Up @@ -319,7 +319,7 @@ fn publish_org_owner_owned() {
);
}

// Test trying to publish a krate we do own (but only because of teams)
/// Test trying to publish a krate we do own (but only because of teams)
#[test]
fn publish_owned() {
let (app, _) = TestApp::full().empty();
Expand All @@ -340,7 +340,7 @@ fn publish_owned() {
user_on_one_team.publish_crate(crate_to_publish).good();
}

// Test trying to change owners (when only on an owning team)
/// Test trying to change owners (when only on an owning team)
#[test]
fn add_owners_as_org_owner() {
let (app, _) = TestApp::init().empty();
Expand Down
89 changes: 40 additions & 49 deletions src/tests/user.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,16 +374,15 @@ fn updating_existing_user_doesnt_change_api_token() {
assert_eq!("bar_token", user.gh_access_token);
}

/* Given a GitHub user, check that if the user logs in,
updates their email, logs out, then logs back in, the
email they added to crates.io will not be overwritten
by the information sent by GitHub.

This bug is problematic if the user's email preferences
are set to private on GitHub, as GitHub will always
send none as the email and we will end up inadvertently
deleting their email when they sign back in.
*/
/// Given a GitHub user, check that if the user logs in,
/// updates their email, logs out, then logs back in, the
/// email they added to crates.io will not be overwritten
/// by the information sent by GitHub.
///
/// This bug is problematic if the user's email preferences
/// are set to private on GitHub, as GitHub will always
/// send none as the email and we will end up inadvertently
/// deleting their email when they sign back in.
#[test]
fn github_without_email_does_not_overwrite_email() {
let (app, _) = TestApp::init().empty();
Expand Down Expand Up @@ -425,9 +424,8 @@ fn github_without_email_does_not_overwrite_email() {
assert_eq!(json.user.email.unwrap(), "apricot@apricots.apricot");
}

/* Given a new user, test that if they sign in with one email, change their email on GitHub, then
sign in again, that the email in crates.io will remain set to the original email used on GitHub.
*/
/// Given a new user, test that if they sign in with one email, change their email on GitHub, then
/// sign in again, that the email in crates.io will remain set to the original email used on GitHub.
#[test]
fn github_with_email_does_not_overwrite_email() {
use cargo_registry::schema::emails;
Expand Down Expand Up @@ -461,10 +459,9 @@ fn github_with_email_does_not_overwrite_email() {
assert_eq!(json.user.email, Some(original_email));
}

/* Given a crates.io user, check that the user's email can be
updated in the database (PUT /user/:user_id), then check
that the updated email is sent back to the user (GET /me).
*/
/// Given a crates.io user, check that the user's email can be
/// updated in the database (PUT /user/:user_id), then check
/// that the updated email is sent back to the user (GET /me).
#[test]
fn test_email_get_and_put() {
let (_app, _anon, user) = TestApp::init().with_user();
Expand All @@ -480,16 +477,15 @@ fn test_email_get_and_put() {
assert!(json.user.email_verification_sent);
}

/* Given a crates.io user, check to make sure that the user
cannot add to the database an empty string or null as
their email. If an attempt is made, update_user.rs will
return an error indicating that an empty email cannot be
added.

This is checked on the frontend already, but I'd like to
make sure that a user cannot get around that and delete
their email by adding an empty string.
*/
/// Given a crates.io user, check to make sure that the user
/// cannot add to the database an empty string or null as
/// their email. If an attempt is made, update_user.rs will
/// return an error indicating that an empty email cannot be
/// added.
///
/// This is checked on the frontend already, but I'd like to
/// make sure that a user cannot get around that and delete
/// their email by adding an empty string.
#[test]
fn test_empty_email_not_added() {
let (_app, _anon, user) = TestApp::init().with_user();
Expand All @@ -510,12 +506,11 @@ fn test_empty_email_not_added() {
);
}

/* Check to make sure that neither other signed in users nor anonymous users can edit another
user's email address.

If an attempt is made, update_user.rs will return an error indicating that the current user
does not match the requested user.
*/
/// Check to make sure that neither other signed in users nor anonymous users can edit another
/// user's email address.
///
/// If an attempt is made, update_user.rs will return an error indicating that the current user
/// does not match the requested user.
#[test]
fn test_other_users_cannot_change_my_email() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down Expand Up @@ -543,12 +538,11 @@ fn test_other_users_cannot_change_my_email() {
);
}

/* Given a new user, test that their email can be added
to the email table and a token for the email is generated
and added to the token table. When /confirm/:email_token is
requested, check that the response back is ok, and that
the email_verified field on user is now set to true.
*/
/// Given a new user, test that their email can be added
/// to the email table and a token for the email is generated
/// and added to the token table. When /confirm/:email_token is
/// requested, check that the response back is ok, and that
/// the email_verified field on user is now set to true.
#[test]
fn test_confirm_user_email() {
use cargo_registry::schema::emails;
Expand Down Expand Up @@ -585,10 +579,9 @@ fn test_confirm_user_email() {
assert!(json.user.email_verification_sent);
}

/* Given a user who existed before we added email confirmation,
test that `email_verification_sent` is false so that we don't
make the user think we've sent an email when we haven't.
*/
/// Given a user who existed before we added email confirmation,
/// test that `email_verification_sent` is false so that we don't
/// make the user think we've sent an email when we haven't.
#[test]
fn test_existing_user_email() {
use cargo_registry::schema::emails;
Expand Down Expand Up @@ -638,9 +631,8 @@ fn test_user_owned_crates_doesnt_include_deleted_ownership() {
assert_eq!(json.owned_crates.len(), 0);
}

/* A user should be able to update the email notifications for crates they own. Only the crates that
were sent in the request should be updated to the corresponding `email_notifications` value.
*/
/// A user should be able to update the email notifications for crates they own. Only the crates that
/// were sent in the request should be updated to the corresponding `email_notifications` value.
#[test]
fn test_update_email_notifications() {
let (app, _, user) = TestApp::init().with_user();
Expand Down Expand Up @@ -723,9 +715,8 @@ fn test_update_email_notifications() {
})
}

/* A user should not be able to update the `email_notifications` value for a crate that is not
owned by them.
*/
/// A user should not be able to update the `email_notifications` value for a crate that is not
/// owned by them.
#[test]
fn test_update_email_notifications_not_owned() {
let (app, _, user) = TestApp::init().with_user();
Expand Down