Skip to content

Comments

ref: git mv tests/fixtures fixtures#35445

Merged
asottile-sentry merged 2 commits intomasterfrom
asottile-fixtures
Jun 8, 2022
Merged

ref: git mv tests/fixtures fixtures#35445
asottile-sentry merged 2 commits intomasterfrom
asottile-fixtures

Conversation

@asottile-sentry
Copy link
Contributor

I'm moving test code which isn't tests to fixtures/ so I can enforce a naming convention for python test files

this is part of https://getsentry.atlassian.net/browse/DEVINFRA-43

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

size-limit report 📦

Path Size
src/sentry/static/sentry/dist/entrypoints/app.js 62.94 KB (+0.05% 🔺)
src/sentry/static/sentry/dist/entrypoints/sentry.css 69.78 KB (0%)

@@ -0,0 +1 @@
../fixtures No newline at end of file
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a (temporary) symlink -- some stuff in getsentry hardcodes tests/fixtures and I'll have to fix those in a 3-step process

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, classic

from tests.sentry.api.endpoints.test_user_authenticator_details import assert_security_email_sent


# TODO(joshuarli): move all fixtures to a standard path relative to gitroot,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODONE

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:3

@asottile-sentry asottile-sentry marked this pull request as ready for review June 7, 2022 21:47
@asottile-sentry asottile-sentry requested a review from a team June 7, 2022 21:47
@asottile-sentry asottile-sentry requested review from a team as code owners June 7, 2022 21:47
"fixtures",
name,
)
def get_fixture_path(*parts: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What wasn't correct or optimal about this previously? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it left a bunch of ..s in the path, the path wasn't absolute so it could potentially cut off the root early, and it had the old path

@@ -5,7 +5,7 @@ exclude: >
\.map$|
\.map\.js$|
^tests/sentry/lang/.*/fixtures/|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path matches nothing anymore, can remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, will do in a followup!

Copy link
Member

@joshuarli joshuarli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you missed some dirs?

tests/integration/fixtures
tests/relay_integration/lang/javascript/fixtures
tests/sentry/models/fixtures

@asottile-sentry
Copy link
Contributor Author

Looks like you missed some dirs?

tests/integration/fixtures
tests/relay_integration/lang/javascript/fixtures
tests/sentry/models/fixtures

yeah, I'm doing things one at a time

@asottile-sentry asottile-sentry merged commit 454a949 into master Jun 8, 2022
@asottile-sentry asottile-sentry deleted the asottile-fixtures branch June 8, 2022 18:19
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2022
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.

2 participants