- 
                Notifications
    
You must be signed in to change notification settings  - Fork 72
 
fix: allow steps.get in world-postgres to handle undefined runId #135
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.
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
fix: allow steps.get in world-postgres to handle undefined runId #135
Conversation
          🦋 Changeset detectedLatest commit: 9f2461b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
 Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR  | 
    
| 
           @eersnington is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it.  | 
    
| 
           The latest updates on your projects. Learn more about Vercel for GitHub. 
  | 
    
| 
           @eersnington awesome, thank you for the fix! Any chance we can also add a test that would ensure this behaviour? We could probabaly do it in the world-testing package, or in the PG world directly?  | 
    
          
 thank you! I saw that local world has tests for this. I'm leaning towards world-testing since it would catch this across all storage implementations per the interface contract. But, one problem that I can think of is world-testing shouldn't be too tightly coupled to backend specific details. Maybe a generic contract test + specific edge cases in each world package? (I haven't gone through all the tests in local world, but I think that can act as a starting point) Would love to hear your thoughts and the team's on this. I'd be happy to take it on too!  | 
    
| 
           @eersnington world-testing makes sense but I guess to get this PR through without too much friction, I'd suggest copying that same test over from local for now and shipping it a a world-postgres test The world spec itself needs a bunch of work - we're discussing this as a team right now to I think the plan for world-testing is to have a good suite of tests, including but not limited e2e workflow tests, and should include testing backend implementation. For example, things like "You cannot cancel a run that's already complete" and "If you mark a step as completed, the completedAt needs to be updated", etc The creator of worlds is   | 
    
| 
           Are we good to merge this?  | 
    
          
 @VaguelySerious Hey Peter, I've added better tests (my previous commit, I mocked drizzle client which sucked). Now it run against the docker-compose postgres container. Can you review the current ones and point out anything you'd change? I'll add more tests based on the one written by Gal in local-world/storage.test.ts. Rn, there's tests for createRunsStorage, and createStepsStorage, and there's none for createEventsStorage, and createHooksStorage  | 
    
| 
           I think this is already really good and we can merge as-is. Later, when we use world-testing for this too and have more comprehensive tests there, it'll likely replace the storage tests anyway.  | 
    
| 
           Sorry we needed to revert this because of a license change @eersnington Can you please reopen the PR on the latest  When making a new PR, you'll need to signoff the commit using the  Apart from that, the PR itself looks great. Happy to answer any questions on the licensing if you're curious  | 
    
          
 Thank you @VaguelySerious! I've just finished writing all the tests locally for storage.ts basing it of Gal's tests for Embedded world and found 1 problem too within the storage contract for PG World through the tests.  
^ Would definitely be worth it to write comprehensive integration tests to find these details across all world implementations 
 No worries @pranaygp and thank you for letting me work on this 🫡  | 
    

World-postgres steps.get was always expecting runId, so passing undefined returned 404.
This patch makes it fall back to stepId-only lookups, like the other backends (world-local).
Closes #134