-
Notifications
You must be signed in to change notification settings - Fork 11
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
tests: Don't start DotcomWeb.Endpoint as a server in tests #2180
Conversation
Which test checks that value? We can probably remove it too. |
dotcom/test/cms/paragraph_test.exs Lines 323 to 328 in 6ba2846
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, had no idea about the test. I'm in favor of removing the hardcoded localhost:4002
from the relevant test.
There are a few tests that reference localhost:4002. Pretty sure they are just looking at URLs. Not sure they actually expect anything to be running. If it's just checking for a URL we can probably remove the localhost part and just use |
0431f4b
to
0b5329a
Compare
@anthonyshull @thecristen - I removed a bunch of hard-coded |
Summary of changes
Start
DotcomWeb.Endpoint
without theserver: true
flag in tests.Starting
DotcomeWeb.Endpoint
withserver: true
causes the tests to fail if anything is already running on the port supplied (in this case4002
). That conflicts with the docker config, which runsdotcom-1
on the same port, so if you try to runmix test
while the docker setup is running, they will reliably fail.Completely removing
DotcomWeb.Endpoint
runs afoul of one test, which haslocalhost:4002
hard-coded in its assertion, which is why I didn't completely obliterate it theDotcomWeb.Endpoint
invocation. Another option, which I'm open to, would be to remove the hard-codedlocalhost:4002
from that test.General checks
New UI, or substantial UI changes
New endpoints, or non-trivial changes to current endpoints