-
Notifications
You must be signed in to change notification settings - Fork 76
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
Handle host ports with emulator tests #1464
Handle host ports with emulator tests #1464
Conversation
641111a
to
831a2a9
Compare
61f56f4
to
49fb30b
Compare
also make sure to randomize host port so that tests don't clobber each other when ran in parallel
49fb30b
to
cba0bdc
Compare
045e905
to
a15e7c7
Compare
a15e7c7
to
01b0096
Compare
The instability of tests seems to still be there 😵💫😵💫
Good thing is that it's only |
Bummer! Thanks for checking this out - I am struggling to repro this locally, even when running the test a bunch of times in short order. 🤔 Looking at it again this morning - it looks like we are waiting for the UI port to be running, but are not running for the emulated device itself. I will push an update for that shortly. |
Still get the failures on the latest HEAD...
Seems to be passing on CI though 😅 |
Ran the test 30 times locally, the only flaky test is |
I don't know why I didn't see these failures before, but i am seeing it now! I think what's happening now is that even though I was checking if the port is in use, I was checking at 127.0.0.1, but it looks like docker is publishing ports at all network interfaces, not just 127.0.0.1. I'll push up a change shortly to check if the port is open on all interfaces (0.0.0.0). Oh yeah, and why that test is so special... I wonder if it is just the order in which the tests are run, that one tends to be the first one to reuse a port that was already in use? Not 100% sure. |
ebfa99b
to
c1f8951
Compare
I was actually trying to run just this test before (in isolation), and it was still flaky (I was also thinking that maybe there was some environment issue). It was before the changes introduced in this PR though |
This reverts commit 01b0096.
a5bcc72
to
3749430
Compare
I was able to get this to pass a few times in CI on my fork, with the workflow running on a couple of branches at the same time. Are we comfortable merging this in, and then I can reenable and test it out for real? @Ifropc @leighmcculloch If we start seeing issues again, we can disable again, and reevaluate. |
Troubleshooting emulator tests
Closes #1461 🤞
What
We were still seeing emulator test failures.
Why/Issue Description
The speculos emulator dockerfile exposes a bunch of ports that we don't necessarily need for our tests. We relied on testcontainer's port publishing, which publishes all ports by default. I think that there were conflicts in which ports were being used for which tests. Since the tests are run in parallel, sometimes a test would try to access the API/UI endpoint on the GH runner host, and another test container was also using that port. 😵
This PR explicitly only publishes the two ports that we need, and makes sure to get two open ports per test.
Known limitations
We are on an older version of testcontainers, which we can hopefully update as part of #1471. I'm just pointing this out here, because our current version of testcontainers doesn't make it easy to publish just the ports with want, at dynamic ports on the host. Maybe we can clean this up when testcontainers is updated.
Shout out to @Ifropc for digging into this too!