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

Explain the purpose of the "tests" conditional dependency requirement#7751

Merged
anoadragon453 merged 3 commits into
developfrom
anoa/jwt_tests
Jun 30, 2020
Merged

Explain the purpose of the "tests" conditional dependency requirement#7751
anoadragon453 merged 3 commits into
developfrom
anoa/jwt_tests

Conversation

@anoadragon453
Copy link
Copy Markdown
Member

@anoadragon453 anoadragon453 commented Jun 28, 2020

jwt, provided the the pyjwt module, is required by our unit tests

This PR adds the module to the test conditional requirement.

This does mean that we have to change a version number in two places if we ever want to update pyjwts minimum version. Is there a way to consolidate this? (I'm aware I could define a version string outside of the CONDITIONAL_REQUIREMENTS dict, but is there a better way?)

EDIT: This PR has been updated to explain that "test" is not supposed to be all of the modules necessary to run the unit tests, but rather those modules that are exclusively required by unit test code.

@anoadragon453 anoadragon453 requested a review from a team June 28, 2020 12:13
@clokep
Copy link
Copy Markdown
Member

clokep commented Jun 29, 2020

I think in the past the assumption has been that tests get run with matrix-synapse[all] install? Not sure if that is valid though.

@anoadragon453
Copy link
Copy Markdown
Member Author

anoadragon453 commented Jun 29, 2020

Hm, if that's the case we should probably place a comment in python_dependencies.py that states the "test" conditional refers to modules that are only required by tests, but are not all those that are necessary for testing.

Edit: This PR has been updated to perform the above.

@anoadragon453 anoadragon453 changed the title Add the pyjwt package to test requirements Explain the purpose of the "tests" conditional dependency requirement Jun 29, 2020
Copy link
Copy Markdown
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.

given that all implies test... what is the point of the test extra?

@clokep
Copy link
Copy Markdown
Member

clokep commented Jun 29, 2020

Probably just to avoid magic in the calculation of the "all" requirement?

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Jun 29, 2020

fair

Copy link
Copy Markdown
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.

lgtm!

@anoadragon453 anoadragon453 merged commit 2f6afdd into develop Jun 30, 2020
@anoadragon453 anoadragon453 deleted the anoa/jwt_tests branch June 30, 2020 09:11
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'dc80a0762':
  1.16.0rc1
  Back out MSC2625 implementation (#7761)
  Additional configuration options for auto-join rooms (#7763)
  Add some metrics for inbound and outbound federation processing times (#7755)
  Explain the purpose of the "tests" conditional dependency requirement (#7751)
  Add another yield point to state res v2 (#7746)
  Move flake8 to end. Don't exit script on failure (#7738)
  Make tox actions work on Debian 10 (#7703)
  Yield during large v2 state res. (#7735)
  add org.matrix.login.jwt so that m.login.jwt can be deprecated (#7675)
  Set Content-Length for Metrics requests (#7730)
  Sync ignored table names in synapse_port_db to current database schema (#7717)
  Allow local media to be marked as safe from being quarantined. (#7718)
  Convert directory handler to async/await (#7727)
  Speed up state res v2 across large state differences. (#7725)
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