Skip to content
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

test scaffolding and DO storage instrumentation tests #35

Open
wants to merge 188 commits into
base: main
Choose a base branch
from

Conversation

rtbenfield
Copy link
Collaborator

This PR introduces Vitest with initial unit tests covering Durable Object storage instrumentation. These tests are focused on verifying the resulting spans match the expected behavior.

In writing these tests, I found that if Durable Object storage calls fail then the spans are not captured. The related tests are skipped until implementing a fix in a follow-up PR.

@rtbenfield
Copy link
Collaborator Author

It seems like the GitHub Actions pipeline isn't running since this is coming from a fork. I thought pull_request_target should enable that, but maybe it has to be on main first 🤔 @evanderkoogh do you know if there is a different trigger needed here?

@evanderkoogh
Copy link
Owner

I have never used GitHub actions to be fair, but I do know that you really don't want to be automatically building/running code from forks, because people will just mine the latest shitty crypto coin from your account.

But I have added you to the repository, so you should be able to test this for yourself now :)

@DaniFoldi
Copy link
Contributor

Workflows triggered by pull_request_target use the workflow in the base branch, which doesn't contain the workflow file yet. I agree with @evanderkoogh that running code from forks introduces a security risk, so best way is to let a maintainer write the action in an internal PR.

(Docs here: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)

@evanderkoogh
Copy link
Owner

Hey @rtbenfield, I had to do some repository dark magic to remove a file that shouldn't have been in the repository. It would probably be easiest to redo this PR?

@rtbenfield
Copy link
Collaborator Author

Hey @rtbenfield, I had to do some repository dark magic to remove a file that shouldn't have been in the repository. It would probably be easiest to redo this PR?

Yeah no problem! I have the changes in my own branch so I can rebase and raise a new PR.

@evanderkoogh
Copy link
Owner

Hey @rtbenfield, do I need to keep this PR around still? Would love to get this merged and get some tests in here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants