-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update TESTING.md #694
base: main
Are you sure you want to change the base?
Update TESTING.md #694
Conversation
Added Guardrails test conditions for JB client
TESTING.md
Outdated
|
||
### Guardrails: | ||
|
||
- [ ] Check if Guardrails check passes for any code longer than 10 lines generated in Cody Chat |
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.
Hey, thank for adding test scenario for Guardrails but it looks a bit incomplete to me.
It does not explain what Guardrails check
is and how to check if it passed.
Ideally it should be in the form:
- Do to trigger some action
- Check if results looks like that:
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 tried to put in a detailed description of actions, and tangible expectation. Let me know if that works OK.
@pkukielka Does this loom help explain the feature and I can accommodate that into the test plans: https://www.loom.com/share/077cf0f2b6174c5dbd2825653f6ed4cd?sid=6f76b12b-afd6-4181-802d-2eaa0c35b005 |
Unfortunately I do not have access to this video. |
@pkukielka sharing the video through GDrive |
Thanks, that looks fine.
For me it always returns a bit of code which fails the guardrails. |
@aramaraju are you still working on updates to this PR and sourcegraph/cody#3204? |
@taylorsperry looks like @pkukielka was able to find a test to verify this. I'm good with that to check for testing for the beta version of it |
@aramaraju I think it would be good to have this as part of our |
+1 to @pkukielka 's comments about making the test plan literal with specific actions and expected outputs. It's not going to scale to have a 4 minute Loom for every feature. |
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.
Putting this back in @aramaraju 's queue for @pkukielka 's feedback.
@cbart Can you suggest a few test cases that you used with the JBs team? You know better the OSS code that's indexed. Thank you :) |
@aramaraju @cbart this is hanging unmerged for 2 months now. |
Apologies for not responding more promptly here. I did not see the notifications. I'll work through this today. |
Done now. The test only goes through success scenario (guardrails check passes). Testing a guardrails-failed scenario for a generated snippet proves to be extremely hard in the regular use case. Even when giving Cody exact instructions to generate - really rewrite a given snippet it always seems to adjust indentation or make some tiny changes here and there - and attribution check is an exact match. The way we've been doing this so far was to actually put a snippet in Cody chat ourselves (guardrails runs there too!) and then we have full control over how the snippet looks like, but in both editors that's not possible right now (link). |
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.
LGTM
@aramaraju should we close this out? |
Looks like we capture the tests here from Cezary's changes, I'm okay to close it or merge it |
Cool, @aramaraju I'm going to approve so I stop getting the notifications, but defer to you on next steps :) |
Added Guardrails test conditions for JB client
Test plan
Documentation change -reading through and review.