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

Add test for symbolicator pipeline #1916

Merged
merged 15 commits into from
Jan 19, 2023

Conversation

emmatyping
Copy link
Contributor

Fixes #1888

This integration test:

uploads a symbol file, which indicates where to find the symbols for the minidump
uploads a sample minidump, which creates an event we can check for

This test is based on the test here: https://github.com/getsentry/sentry/blob/28f4f36b4adc3d39ea80f2f1864cff8e311b6af3/tests/symbolicator/test_minidump_full.py#L72

@emmatyping emmatyping force-pushed the ethanhs/symbolicator-integration-test branch from 9c4f415 to bbeae53 Compare January 17, 2023 23:27
@emmatyping emmatyping marked this pull request as ready for review January 17, 2023 23:28
_integration-test/run.sh Show resolved Hide resolved
_integration-test/run.sh Show resolved Hide resolved
@chadwhitacre
Copy link
Member

Passing case. 💃

Let's see it catch a failure! 😁

@BYK
Copy link
Member

BYK commented Jan 18, 2023

Observation: I don't recall why I opted for using bash for these tests (probably for something quick and dirty) but at this point, it feels to me that the tests should be a bit more organized and manageable? Maybe using something like Playwright?

@emmatyping
Copy link
Contributor Author

@BYK eventually I'd like to rewrite the installer in Python and make it so we can use pytest and all the creature comforts that come from that (probably use playwright in tests as well!). But that is a much larger project 😅

@emmatyping
Copy link
Contributor Author

emmatyping commented Jan 18, 2023

Here is a sample of a failure: https://github.com/getsentry/self-hosted/actions/runs/3952966281/jobs/6768689917#step:5:6808

Symbol upload will fail of course, so we fail at the sentry-cli point.

EDIT: failing commit now reverted

@BYK
Copy link
Member

BYK commented Jan 18, 2023

@BYK eventually I'd like to rewrite the installer in Python and make it so we can use pytest and all the creature comforts that come from that (probably use playwright in tests as well!). But that is a much larger project 😅

This is kind of orthogonal to what I'm suggesting though. There's no reason for integration tests to be in the same language or environment with the installer (maybe it was my faulty brain which fell into this misconseption in the first place, don't really remember). We can right now at this moment, start creating Playwright tests and start benefitting from them which would cut through most of this crazy bash/curl/jq dance with CSRF tokens etc. It would:

  • Cover more surface area
  • Be easier to understand and maintain
  • Have videos of the failures if we wanted to
  • Have screenshot tests if we wanted to

Regarding rewriting the installer in Python (or any other language tbh), I'd suggest finding ways of getting rid of it completely (such as by adopting Ansible, Helm charts, or whatever). It would be a challenge to use Python unless the installer gets compiled into a self-contained binary for each system by using something like PyOxidizer

emmatyping added a commit to getsentry/action-self-hosted-e2e-tests that referenced this pull request Jan 18, 2023
This is a new dependency of the integration tests from getsentry/self-hosted#1916
@chadwhitacre
Copy link
Member

Also benefiting from #1762: 😁

self-hosted-1-symbolicator-1                               "/bin/bash /docker-e…"   symbolicator                               exited (137)        

@chadwhitacre
Copy link
Member

I'm open to Playwright, def separate from installer lang.

@emmatyping
Copy link
Contributor Author

emmatyping commented Jan 19, 2023

We can right now at this moment, start creating Playwright tests and start benefitting from them which would cut through most of this crazy bash/curl/jq dance with CSRF tokens etc.

This is true. I am hesitant to start on this since we are near the end of the quarter and I don't want the project delayed much more, but maybe when I add the snuba tests I will add them as playwright tests.

Regarding rewriting the installer in Python (or any other language tbh), I'd suggest finding ways of getting rid of it completely (such as by adopting Ansible, Helm charts, or whatever). It would be a challenge to use Python unless the installer gets compiled into a self-contained binary for each system by using something like PyOxidizer

The idea with that was to make the installer run entirely in docker except for a minimal kickoff script. I agree Ansible might be better, but I'm unsure if making that a dependency is acceptable for our users (right now we only require GNU core utils and docker).

@emmatyping emmatyping merged commit 19d6edf into master Jan 19, 2023
@emmatyping emmatyping deleted the ethanhs/symbolicator-integration-test branch January 19, 2023 20:57
@chadwhitacre chadwhitacre mentioned this pull request Jan 25, 2023
14 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test for symbolification pipeline, based on test in Sentry repo
4 participants