Skip to content

Use UserBuilder in more tests - #14238

Merged
Turbo87 merged 3 commits into
rust-lang:mainfrom
carols10cents:user-builder-more
Jul 16, 2026
Merged

Use UserBuilder in more tests#14238
Turbo87 merged 3 commits into
rust-lang:mainfrom
carols10cents:user-builder-more

Conversation

@carols10cents

Copy link
Copy Markdown
Member

As we remove gh_* fields from user records, using the UserBuilder in more places now means fewer places will need to be updated to remove the gh_* fields when we deprecate those since the UserBuilder consolidates setting the gh_* fields on the test user records to one place.

There are still a few places that I've deliberately not used UserBuilder, such as:

  • sync admins test; we only want to support setting is_admin in the sync admins job, not the regular user flow
  • tests deliberately exercising what happens when users.gh_id is -1, which is still a thing in production

Comment thread crates/crates_io_database/Cargo.toml Outdated
[dev-dependencies]
claims = "=0.8.0"
crates_io_test_db = { path = "../crates_io_test_db" }
crates_io_test_utils = { path = "../crates_io_test_utils" }

@Turbo87 Turbo87 Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... I'm feeling a bit uneasy about adding this to the database crate since it pulls in:

crates_io_api_types = { path = "../crates_io_api_types" }
crates_io_cargo_toml = { path = "../crates_io_cargo_toml" }
crates_io_database = { path = "../crates_io_database" }
crates_io_encryption = { path = "../crates_io_encryption" }
crates_io_linecount = { path = "../crates_io_linecount" }
crates_io_tarball = { path = "../crates_io_tarball", features = ["builder"] }

that creates quite a spaghetti in the dependency tree 😅

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 🤷🏻‍♀️ i put it back the way it was!

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

View changes since this review

@@ -22,6 +22,7 @@ static ENCRYPTED_TOKEN: LazyLock<Vec<u8>> = LazyLock::new(|| {
/// If you want to test logic that happens as part of signing up or logging in,
pub struct UserBuilder<'a> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering at what point we would want to pull bon in, but probably not at two fields.

id: 1,
gh_login: self.username.into(),
name: Some("The Octocat".into()),
name: self.display_name.map(ToString::to_string),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a change in behaviour, but since the CI is green I'm assuming we weren't relying on it anywhere.

@Turbo87
Turbo87 merged commit 1538a65 into rust-lang:main Jul 16, 2026
12 checks passed
@carols10cents
carols10cents deleted the user-builder-more branch July 16, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants