Conversation
randycoulman
left a comment
randycoulman
left a comment
There was a problem hiding this comment.
This seems OK, but I'm not sure the registry clearing code is doing what you intend here. See my note below.
Based on my understanding of how Registry and start_supervised work, the only required change here seems to be making ConfigCatTest be async: false. I think the rest of the changes could safely be reverted, including the combining of two separate test files.
But you may be seeing something I'm not.
| defp clear_registry(_context) do | ||
| ConfigCat.Registry | ||
| |> Registry.match({__MODULE__, :_}, :_) | ||
| |> Enum.each(fn {key, _} -> Registry.unregister(ConfigCat.Registry, key) end) | ||
|
|
||
| :ok | ||
| end |
There was a problem hiding this comment.
Is this part strictly necessary? Processes should remove themselves from the Registry when they shut down, and the use of start_supervised! ensures that processes are shut down when the test ends.
If it is necessary, are you sure that this code is doing what you want? __MODULE__ in the match spec will refer to this test's module (ConfigCat.IntegrationTest), but all of the started processes will have the module as ConfigCat.Supervisor, if I'm remembering correctly.
There was a problem hiding this comment.
My first attempt was to simply turn off async test execution in ConfigCatTest. But on GitHub I still got the error. After that, I implemented the clear_registry function to ensure that the previous ConfigCat client instance is removed from the Registry. If I remember correctly, even with these changes, I occasionally encountered the error (though less frequently). Finally, I combined the two separate tests, and the error disappeared.
There was a problem hiding this comment.
I'm pretty sure that clear_registry isn't doing anything (because __MODULE__ is different in the test than it is in the code), but if this is working for you, feel free to merge it.
There was a problem hiding this comment.
You are right @randycoulman, I also got the error with the combined version (less frequently, but I think it is just because of the timing).
How should I use start_supervised! to ensure the process is shut down when the test ends?
There was a problem hiding this comment.
From what I can see, we're already using start_supervised in start_config_cat, which is what every test calls. Clearly something isn't shutting down fast enough, though. I have a different idea to try. I'll open up a draft PR once I have it working.
There was a problem hiding this comment.
@kp-cat See #143 for an alternate solution for this test problem.
If we find that the tests continue spuriously failing, we could change our CI config to run mix coveralls.json --warnings-as-errors --max-cases 1. That would force the tests to all run serially. That'll make the build a bit slower, but should make the problem go away.
There was a problem hiding this comment.
Thanks @randycoulman,
I would avoid extra dependencies just because of this issue. I prefer the --max-cases 1 solution. I changed the CI config slightly to run the integration tests separately. What do you think about these changes here?
kp-cat
force-pushed
the
fix-test
branch
4 times, most recently
from
e5c2b70 to
6fdaad6
Compare
randycoulman
left a comment
randycoulman
left a comment
There was a problem hiding this comment.
As mentioned below, --max-cases 1 isn't doing anything different than what ExUnit does on its own. What happens when you run the entire test suite with --max-cases 1 instead? If that works, it would simplify the changes you had to make to merge the coverage information.
That said, if this separation is actually solving the problem, you should go ahead and merge it.
| - name: Execute tests | ||
| run: mix coveralls.json --warnings-as-errors | ||
| run: | | ||
| mix test --only integration --warnings-as-errors --max-cases 1 --cover --export-coverage integration-coverage |
There was a problem hiding this comment.
--max-cases 1 has no effect here. ExUnit will run async: true modules in parallel with each other, but runs the tests within each module in series. Since there is only one module (aka case) with the integration tag, there will only be one case being run no matter what.
There was a problem hiding this comment.
You right but If I remove --max-cases 1, it seems the integration test can be broken again 😒
I'll just keep it like this.
This reverts commit 877ed28.
3 participants
Describe the purpose of your pull request
Clear the ConfigCat Registry before every testrun to fix broken async integration tests:
warn [3000] There is an existing ConfigCat instance for the specified SDK Key. No new instance will be created and the specified options are ignored. You can use the existing instance by passing
client: 172e9daa-266f-48bb-bf32-8c4c4c2c2797to the ConfigCat API functions. SDK Key: 'configcat-sdk-1/PKDVCLf-Hq-h-kCzMp-L7Q/1cGEJXUwYUGZCBOL-E2sOw'.Requirement checklist (only if applicable)