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

Decouple service from global state #186

Merged
merged 21 commits into from
Nov 1, 2023
Merged

Decouple service from global state #186

merged 21 commits into from
Nov 1, 2023

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Oct 6, 2023

This PR effectively refactor the service to stop depending on global state, which allows us to start testing the service in it's component pieces. Quite a lot of the codebase depended on the global config, which means we'd have to mock it every time in order to run tests. This PR fully separates out the code from the config which will allow us to start testing,

Review Guide

I'm afraid this is fairly wide ranging, so I'll try to explain my changes:

  • The index.ts file now creates a class ConferenceBot, which retains the true global state of everything.
  • We now have a ConferenceMatrixClient that extends MatrixClient with a prepared Identity Client to allow early failure.
  • The config is no longer global, but passed between the major classes.
  • The command handlers are now constructed with their dependencies, rather than passed them via doAction for a cleaner API.
  • There is now the beginnings of a test harness that can create a homeserver on homerunner and run through some tests for us.
  • Conference bot does a best effort to clean up on closure, rather than exiting immediately.

@Half-Shot Half-Shot marked this pull request as ready for review October 11, 2023 23:35
@Half-Shot
Copy link
Contributor Author

I think this is probably ready for a first pass review.

@reivilibre
Copy link
Contributor

reivilibre commented Oct 31, 2023

The config is no longer global, but passed between the major classes.

thank you so much, the global config bothered me a lot but I didn't know if it was just conventional in node apps or so, or what the right fix was (I'm not a JS/TS developer by trade)

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

main changes seem nice and straightforward, albeit large.

Testing machinery seems a bit fiddly but overall fine.

.github/workflows/e2e-test.yml Outdated Show resolved Hide resolved
spec/util/e2e-test.ts Outdated Show resolved Hide resolved
@Half-Shot Half-Shot merged commit 4e4aec3 into main Nov 1, 2023
4 checks passed
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.

2 participants