-
Notifications
You must be signed in to change notification settings - Fork 390
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
Use multiple Tigris instances to run tests #1878
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1878 +/- ##
==========================================
- Coverage 66.11% 65.95% -0.17%
==========================================
Files 312 313 +1
Lines 15230 15240 +10
==========================================
- Hits 10070 10051 -19
- Misses 4163 4194 +31
+ Partials 997 995 -2
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Tigris tests faster than PostgreSQL? 😆 I guess it's because of number of skips
LGTM!
// getNextTigrisPort gets the next port number of Tigris to be used | ||
// for testing in Round Robin fashion. |
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.
Nice usage!
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.
I like the idea! Asked a question about configuration.
integration/setup/startup.go
Outdated
) | ||
|
||
// ports are available port of Tigris. | ||
var ports = []uint16{8081, 8082, 8083, 8085, 8086} |
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.
I like this idea, but I'm a bit skeptical about these ports being defined here as the exact number of exact ports and not being configurable.
Is there a plan to make those things configurable (like tigris-url
itself) - maybe in separate PRs?
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.
I've looked at it briefly and decided I would not do it for this, currently there were several places where ports are defined, env tool, docker-compose.yml, and within integrations tests. So it's nice to have configurable but also it touches more than the integration test parameter. For this PR, I'm gonna do quick win. 🙈
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.
Oh, envtool
is a nice catch as we wait for 8081
only, and not for the other ports...
I'm ok to merge it as is to have a quick win, and maybe we can reconsider this and make it configurable after #1857 (so, it'll be easier to deal with configuration).
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.
I think we should start paying a bit more attention to our code comments and not leave explanations on GitHub only, where they are unlikely to be found.
In this case, cross-linking those ports to the Docker Compose configuration would significantly improve understanding.
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.
Yeah agree. I've added comments. I also foresee the number of instances are just a guess and need adjusting hence a new issue #1887
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.
See bd05d83
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.
👍
cmd/envtool/envtool.go
Outdated
if err != nil { | ||
return err | ||
} | ||
// Skip 8084 because since it is used by userland proxy. |
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.
By what "userland proxy"?
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.
It seems to fail only on CI. https://github.com/FerretDB/FerretDB/actions/runs/4040272813/jobs/6945764486
I updated comment to something a bit more understandable.
Error response from daemon: driver failed programming external connectivity on endpoint ferretdb_speedup_3 (331153047b6e2c6218d96d6ac924a2831bdda68f069b8f8a364d00670cc1484f): Error starting userland proxy: listen tcp4 0.0.0.0:8084: bind: address already in use
task: Failed to run task "env-up-detach": exit status 1
8f906a4
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.
👍
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.
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.
LGTM
Use different ports instead of documenting problems. Do not copy&paste the same comment; use references instead. Do not link to closed issue.
bd05d83
Thank you for the clean up @AlekSi 🤗 |
Description
Closes #1568.
Use 4 additional instances for running tests for tigris exposed on ports 8082, 8083, 8085 and 8086. Skipping 8084 because it's used by something else.
On CI, this change appears to runs tigris tests in 6 to 7 minutes. I checked recent merges to main which tigris tests appear to run in 10 to 11min. I don't know how much more $$ it might use.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Assignee, Labels, Project and project's Sprint fields.