Skip to content

Conversation

abcxff
Copy link
Contributor

@abcxff abcxff commented Jan 22, 2025

Changes

Copy link
Contributor Author

abcxff commented Jan 22, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from c562f66 to 7c0e8aa Compare January 22, 2025 21:11
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from 0faf1d5 to eef480c Compare January 22, 2025 21:11
Copy link

cloudflare-workers-and-pages bot commented Jan 22, 2025

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0964bef
Status: ✅  Deploy successful!
Preview URL: https://6426e848.rivet.pages.dev
Branch Preview URL: https://01-22-feat-users-user-wf-tes.rivet.pages.dev

View logs

@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from 7c0e8aa to c70fe90 Compare January 22, 2025 22:19
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch 2 times, most recently from 883775d to 49cc103 Compare January 22, 2025 22:42
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from c70fe90 to e62fbcd Compare January 28, 2025 18:50
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from 49cc103 to c7df546 Compare January 28, 2025 18:50
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from e62fbcd to f129825 Compare January 28, 2025 19:50
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from c7df546 to ba97929 Compare January 28, 2025 19:51
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from f129825 to af1c2e9 Compare January 28, 2025 19:59
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from ba97929 to 8ed4f56 Compare January 28, 2025 19:59
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from af1c2e9 to 1854f2f Compare January 28, 2025 22:35
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch 3 times, most recently from 1215ed1 to 6d9b376 Compare January 29, 2025 03:04
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from 1854f2f to 65dbb93 Compare February 6, 2025 21:51
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from 6d9b376 to 9277340 Compare February 6, 2025 21:51
@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from 65dbb93 to d1e1fb6 Compare February 6, 2025 21:57
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from 9277340 to cfaabb0 Compare February 6, 2025 21:57
@abcxff abcxff marked this pull request as ready for review February 10, 2025 19:38
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces a new faker package for generating test data in the Rivet codebase, focusing on user workflow testing. Here's a concise summary of the key changes:

  • Added new faker service package in /packages/services/faker for generating test data with user operations
  • Implemented user workflow tests using the new faker operations for consistent test data generation
  • Modified existing user tests to use ctx.op(faker::ops::user::Input {}) instead of op!([ctx] faker_user {})
  • Added user registry to workflow worker setup for proper workflow integration
  • Updated monolith-workflow-worker to include user workspace dependency

The changes improve test consistency and provide better infrastructure for user-related testing across the codebase.

20 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from d1e1fb6 to fa080dd Compare February 10, 2025 23:08
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from cfaabb0 to 975b507 Compare February 10, 2025 23:08
Copy link
Member

@NathanFlurry NathanFlurry left a comment

Choose a reason for hiding this comment

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

  • remove unneeded use of compat in faker

@abcxff abcxff force-pushed the 01-22-feat_user_workflows_profile_set_signal branch from fa080dd to 277d41a Compare February 14, 2025 14:31
@abcxff abcxff force-pushed the 01-22-feat_users_user_wf_tests branch from 975b507 to 0964bef Compare February 14, 2025 14:31
@abcxff abcxff mentioned this pull request Feb 14, 2025
@abcxff
Copy link
Contributor Author

abcxff commented Feb 14, 2025

@greptileai

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Based on the provided context, I'll focus on the most recent changes that haven't been covered in previous reviews:

This PR continues the implementation of user workflow testing with the following key updates:

  • Added workflow test in /packages/services/faker/tests/user.rs to verify basic user creation functionality
  • Implemented user workflow registration in /packages/services/user/src/lib.rs for proper workflow integration
  • Enhanced user workflow module in /packages/services/user/src/workflows/user/mod.rs with improved logging and activity tracking
  • Added helper functions for generating display names and IDs with proper randomization

The changes demonstrate a focus on improving test infrastructure and workflow management for user-related operations.

20 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -0,0 +1 @@
pub mod user;
Copy link

Choose a reason for hiding this comment

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

style: ensure user module is properly documented with rustdoc comments explaining its purpose and usage

Comment on lines +8 to +10
[dependencies]
chirp-workflow.workspace = true
user.workspace = true
Copy link

Choose a reason for hiding this comment

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

style: Dependencies look correct based on usage in tests, but consider adding util/faker as a dependency since it's used in the user tests (e.g. util::faker::email())

use chirp_workflow::prelude::*;

#[workflow_test]
async fn empty(ctx: TestCtx) {
Copy link

Choose a reason for hiding this comment

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

style: function name 'empty' is not descriptive of what the test actually does

Comment on lines +15 to +18
// TODO: Remove compat once ctx.workflow, ctx.subscribe exist on OpCtx
let mut creation_sub = chirp_workflow::compat::subscribe::<
user::workflows::user::CreateComplete, _
>(&ctx.op_ctx(), ("user_id", user_id)).await?;
Copy link

Choose a reason for hiding this comment

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

logic: subscription is created before workflow dispatch but not cleaned up if workflow dispatch fails. Consider using a try-finally pattern or RAII to ensure subscription cleanup.

.dispatch()
.await?;

creation_sub.next().await?;
Copy link

Choose a reason for hiding this comment

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

style: no timeout on creation_sub.next() could potentially hang indefinitely if workflow fails silently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants