-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: ws1 1455 e2e test for get events #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Some minor comments. I'd like to merge your changes in rust-ceramic and then make sure these pass, the 404s make sense right now since the new code isn't deployed.
import { base64 } from 'multiformats/bases/base64' | ||
import * as random from '@stablelib/random' | ||
|
||
export function generateRandomEventId(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe want to update the fast/events.ts
random event generation to use these functions too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Don't think it should be in this PR though cause it does not have anything to do with the e2e test for fetching events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just be a minor refactor though? Shouldn't add too much to this PR's scope and the code comes out cleaner versus someone going in afterwards in a separate PR just for reducing code duplication. WDYT?
I don't have a strong opinion though, it's cool to open a ticket for cleanup, maybe tagged as debt
.
Deciding whether to change adjacent code in a PR is more of an art than a science. In PRs with a lot of moving parts, unrelated changes can certainly be a distraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a separate issue for the clean-up. I am planning to move all re-usable functions (not too many) from event.rs to the utils.
Can save us some time in the future :)
https://linear.app/3boxlabs/issue/WS1-1475/clear-some-technical-debt-for-rust-ceramic-and-cermic-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, ticket looks good. I agree with Mohsin that there is a lot of nuance to refactors. In this case, we're still adding and changing these tests, so I don't think it's a problem to follow up.
My general thinking is this: as long as it doesn't blow up the testing scope, cause breaking changes, make review a lot harder, take way longer, or cause difficult merge conflicts, it's worth doing now. For the review piece, putting those refactoring changes (e.g. moving code across files or renaming) in their own commits can make it easier.
Basically, my preference is to do any refactoring I can when I'm in the code because who knows when I'll be back and have the chance, and there's the overhead of reloading the context into my brain and getting another review etc. It's nice to whittle a little tech debt every PR if possible. I think it's harder to sell "our cycle is tackling tech debt" than "we're doing these features and these tickets are a few extra points so we can take care of things" to leadership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, on addition it might be better to remove debt on the file being touched in the PR, this might be a more standardized way of removing debt. Since a developer has visibility of the file being worked on, introducing refactoring to another file that is not of interest to the PR might increase scope in review rather than having a predefined scope for the task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚀
Mind updating the summary? It says 500 instead of 200 for successful test, and the picture upload got stuck, so you can try that again or just delete that piece.
E2E tests for fetching event data from event_id (string).
Goal is to test the getEvent endpoint end-to-end.
Scenarios tested :
Positive scenario: StatusCode 200 : Publish and event, and then fetch event data for the same event
Negative scenario: StatusCode 404 (Event Not found) : Fetch eventData for an eventId that does not exist
PR testing :
Ran the setup locally :