-
Notifications
You must be signed in to change notification settings - Fork 0
tests? #2
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
Conversation
|
|
||
| // Setup PostHog event monitoring | ||
| function setupPostHogEventMonitoring(page: any) { | ||
| page.on('console', (msg: any) => { |
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.
Lean in on debug output of the SDKs. It's much less flaky than:
- Send the events
- Wait for ingestion
- Query using API
We just care that our client side code still triggers the SDK to send 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.
:oh-yeah: clever!
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.
this is so smart
i do worry each SDK having different debug outputs. maybe we can have a separate end-to-end spec that actually hits the PostHog API that you can run periodically or on demand
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.
We snapshot the first run of each example repo. This isn't by SDK, it's specific down to the project. It shouldn't be flaky (the logic is deterministic for that project + version of SDK). We won't reuse the snapshot and snapshotting logic across different SDKs.
The issue with an end to end spec to PostHog API is that the min-cache refresh time we can set up is 5 minutes 🤣 For general queries like identity, the min refresh period is once every hour. This makes it super hard to do e2e tests.
The SDKs themselves are tested with e2e tests, though. If we need to, we could add e2e cases, but since the SDKs are tested we should be good for now
| @@ -0,0 +1,48 @@ | |||
| { | |||
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.
Snapshots are great for this. We'd need to bump this if debug output changes, but I don't think this is a bad thing.
| // Turn on debug in development mode | ||
| debug: process.env.NODE_ENV === "development", | ||
| // Disable request batching in test environment | ||
| request_batching: false, |
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.
This is really suspect. I plan on updating this to detect if we're in CI and disable batching + allow bot agents with environment variables.
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.
One wrinkle here is that we want to keep this kind of artifact (instrumentation-client.ts) specifically as close to what you'd want in user code as possible, so the robot can cheerfully and safely copy it.
could we do some sort of rewrite of it only during testing, or alter PostHog's behavior through a property set at runtime during 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.
oh derp I imagine that's what you mean by your remarks about environment variables, carry on!
|
|
||
| # env files (can opt-in for committing if needed) | ||
| .env* | ||
| .env |
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.
.env.example is useful to allow
gewenyu99
left a comment
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.
Only app router tests for now. More a RFC to discuss
|
Ohh also did a big stupid, these need to run nightly and I'll setup alerts for us in the.... docs channel for now. I'll try to get to it this week! |
|
yeah, just fired this up, this is awesome, thanks again for taking a swing at this My big thing is that we have to kind of hermetically seal the example code to be exclusively things that are a good idea to cue agents about, or risk contaminating the context with Wrong Ideas. so we should come up with a means either patching init code or patching runtime state, represented in a directory we can then wall off from the MCP-delivered examples. But this would be so thrilling lots of ways. early warning for integration breakage too! |
|
I wonder if we should adopt a convention in this repo for paths that should be excluded by the robot in generating context files. Or perhaps even better, this repo itself should generate the context files, then we'll have full control of how we want to adjust the output. Then all the judgement about which files are included lives HERE, along with the output formatting, instead of in the MCP. seems like a better separation of concerns. |
|
@daniloc I feel like a top level // @ignore-file and inline // WDYT |
|
I do have config that really should be sealed off from the agent here as well. |
this ^ @daniloc we could build, enhance, and experiment with these examples without worry. then when ready, run a |
…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Tests