Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add -prof tox environment #9293

Closed
wants to merge 2 commits into from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Feb 2, 2021

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

@ShadowJonathan
Copy link
Contributor Author

(I think it would also be handy to add this to CONTRIBUTING.md, but i have no idea how to go about that when #9281 is also going on)

@anoadragon453 anoadragon453 requested a review from a team February 2, 2021 11:22
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I'm not really convinced this is worthwhile: it's just extra stuff to maintain, which isn't going to be used very often. I think if you're going to run a profiler, you can run trial yourself.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

.

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Feb 11, 2021

How exactly would this add maintenance load? The only thing I can imagine is a gotcha for "oh, so I can just ignore !prof" when reading tox.ini, this change will only make it so that any test run is able to be profiled (which can be helpful if it's included in the tox config by default), which can then be added to CONTRIBUTING.md. Synapse swapped out plainly running trial with deterministic tox, adding a profiler option to it would only make sense, as every other option has already been folded into tox.

I understand if y'all dont wanna accept this PR, but if you really just wanna put a full dot on that decision, just close it.

@clokep
Copy link
Member

clokep commented Feb 11, 2021

How exactly would this add maintenance load? The only thing I can imagine is a gotcha for "oh, so I can just ignore !prof" when reading tox.ini

tox configs are already confusing enough to read and this adds additional cognitive load each time we need to modify it.

Synapse swapped out plainly running trial with deterministic tox, adding a profiler option to it would only make sense, as every other option has already been folded into tox.

I'm not sure what this means, I think most of the team runs tests directly via trial (and not using tox).

I agree with @richvdh's assessment.

@clokep clokep closed this Feb 11, 2021
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.

3 participants