Skip to content

Conversation

sethfowler
Copy link
Contributor

@sethfowler sethfowler commented Jul 18, 2022

Although we've added some features in the intervening years, most of the relay code was written in 2019, and the state of Go development at that time was much different. Go modules were extremely new and we hadn't yet adopted them. IDE support for Go was in a much different state than it is today. Even our approaches to testing were different.

Right now the relay is receiving renewed focus, and that has made it clear that the local development and testing experience is now out of step with the rest of our code base. This PR makes a number of changes that bring the relay code into more alignment with our modern development approaches, with the aim of improving developer productivity when working on this code. Although there are more improvements we could make down the line, the changes in this PR address a lot of the low hanging fruit:

  • The code base has been transitioned to Go modules. This makes the Go language server much happier; it fixes a lot of spurious errors, and greatly improves the editing experience in Visual Studio Code. Note that, to comply with Go module standards, the source code has been moved out of go/src up to the top level of the repo.
  • Instead of having one large relay_test.go file containing all unit tests, each individual plugin package now has its own set of tests. This is more sustainable as the number of plugins and tests increases. It also fixes a frustrating issue with the previous setup: because of the plugin architecture the relay uses, there was no actual source dependency between the plugins and the tests, so go test could get confused and use a cached version of the test output even when the plugins had changed. With the new arrangement, this won't happen anymore.
  • It's now possible to configure the plugins programmatically, without reading values from the environment or .env. Previously, plugins read environment variables directly, making isolating the tests quite challenging and making it impossible to run the tests in parallel.
  • The plugin interface has been reworked to use a factory-based approach for constructing plugins. This allows a new instance of each plugin to be created for each test, eliminating another issue with isolation between unit tests. This also simplifies the plugin code conceptually, since it's no longer necessary for plugins to handle the situation where they've been created but have not been configured correctly.
  • It's now possible for tests to only load the plugins which are relevant to the test in question; each test no longer tests all the plugins at once.
  • Some helper methods and functions have been added to make it easier to write tests. The catcher and relay services now know their own URLs, so tests no longer need to hardcode them. The catcher service captures the entire last request it received, and makes it available programmatically instead of via a special HTTP endpoint. There's a new helper function that encapsulates the process of setting up and tearing down the catcher and relay, so that tests can focus on the things that are specific to each test.
  • In tests, the relay and catcher servers now listen only on localhost, instead of on all network interfaces. This stops macOS from popping up firewall warning dialogs whenever you run the tests, which was harmless but a bit annoying.

It's a bit unfortunate that all of these changes are bundled into a single PR. It would be possible to tease them apart with enough effort, but there are dependencies between many of these changes - just as one example, the new plugin interface references types exposed by the relay code, but in the pre-Go-modules version of the code base it wasn't possible to do this, because the Go runtime could not tell that the type referenced in the plugin and the type referenced in the core relay code were the same type, so casting the plugin to an instance of the plugin interface would fail. (It's quite possible that there was a way to fix this in a GOPATH-based build, but I couldn't figure out how to do it.) The work involved in separating things out didn't seem worth it in this case, though I'm happy to revisit that if you feel differently @eugene-chang-fs.

Because of the introduction of the factory-based plugin interface and the transition to Go modules, this PR technically makes functional changes, but they're minor; these changes are mostly only visible to developers working on the project.

@sethfowler sethfowler force-pushed the sethf/refactor-to-improve-dev-and-testing branch from 30ddee7 to 19666c3 Compare July 18, 2022 16:11
@eugene-chang-fs
Copy link
Collaborator

I'm fine with things being in this PR rather than being in multiple small ones. I'll try to take a look today.

Copy link
Collaborator

@eugene-chang-fs eugene-chang-fs left a comment

Choose a reason for hiding this comment

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

This LGTM based on what limited knowledge I have on go modules.

I did notice the following problems (I don't think it's introduced by your PR but it is something we need to fix):

Running this immediately seg faults (including running the test with make test in the docker image); unfortunately this is also happening in master so not introduced by this.

The other side is that the tests are never ran (I'm guessing this was initially ran manually by Trevor when he worked on this); I'd like to see that we add it to the make all or similar, but it doesn't need to block this PR.

@eugene-chang-fs
Copy link
Collaborator

I did notice the following problems (I don't think it's introduced by your PR but it is something we need to fix):

Welp my fault for not looking at the other PRs yet, you immediately addressed it in the next PR. Please disregard.

@sethfowler
Copy link
Contributor Author

The other side is that the tests are never ran (I'm guessing this was initially ran manually by Trevor when he worked on this); I'd like to see that we add it to the make all or similar, but it doesn't need to block this PR.

Yeesh, I hadn't noticed that this repo doesn't have a Github Action set up for PR pushes. (build-and-release only runs on tagged branches.) Good callout. I'll address that in a followup.

@sethfowler sethfowler merged commit 7df371a into master Aug 15, 2022
@sethfowler sethfowler deleted the sethf/refactor-to-improve-dev-and-testing branch August 15, 2022 15:14
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