chore: fix failing starknet tests #283
Conversation
WalkthroughThe recent changes involve updates to the expected outputs in the end-to-end (E2E) API tests. Specifically, the string values for two hexadecimal keys have been standardized from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant TestSuite
Client->>API: Request data
API-->>Client: Return data with keys
TestSuite->>TestSuite: Validate response
TestSuite->>Client: Confirm expected output matches actual response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/e2e/api.test.ts (2 hunks)
Additional comments not posted (2)
test/e2e/api.test.ts (2)
103-104: LGTM! Consistent capitalization update.The change to
'Checkpoint'ensures consistency with the updated API response format.
121-122: LGTM! Consistent capitalization update.The change to
'Checkpoint'ensures consistency with the updated API response format.
Unless starknet require all username to be capitalized (which seems doubtful), this bot is just spouting nonsense |
There have been occasions where it made good suggestions but I'd certainly switch a few things off and make them opt-in. This PR moves the config to the repo but I closed most of my PRs to remove noise and focus only on what's urgent for a release. |
There was a problem hiding this comment.
I approve these changes, but this is a good example for the type of PRs I'd merge without approval. It's a purely formal step that doesn't serve its original purpose. I didn't fix the test myself because I wasn't sure what the new correct value is, and I still didn't look that up. @wa0x6e seems to have figured that out and fixed the test correctly. If I had submitted this PR, then @wa0x6e would now approve and merge it. And what's the consequence of a late approval? Currently all other changes are also blocked by these failing tests, even though it's just a small thing. It must be done. What exactly am I supposed to approve here? Should we delete the tests instead? Should I just approve or merge too? Idk.
In cases where the changes:
- contain only very obvious fixes or improvements
- don't go much beyond the complexity of this example
- don't change any behavior (as that mostly needs to be discussed)
I'd open the PR, to send everyone a notification, and then turn it purple right away.
On the other hand, I believe reviews of important feature PRs profit from more extensive discussion and preparation upfront. This is where I'd dogmatically implement a process that ensures ideas have been properly evaluated and technically explored to a degree, that implementation can actually start and in a way others can understand, especially in an open source context where you ideally want external contributors to just pick up work and get it done. For me that doesn't work if I have to come back to a title + one sentence issue after ten minutes of exploring the thing and then wait 3 hours (or forever) for clarification.
I know there's just not much time for this project right now and I simply have too much. But I hope such thoughts will be considered once one or two more devs work on this more regularly, at least if I'm then one of them.
Fix failing starknet E2E tests, due to the account name associated to the address being updated
Summary by CodeRabbit