-
Notifications
You must be signed in to change notification settings - Fork 249
Re-enable GitLabPluginTest #2285
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
base: master
Are you sure you want to change the base?
Conversation
Attempt on optimizing the GitLab Docker image, increased timeouts, and incorporated defensive retries. Wherever possible, use Git directly instead of GitLabApi.
…inutes of inactivity⚠️ But still unreliable, as some CI runs show the GitLab image taking 9 minutes and 40 seconds
…he number of tests
|
I'm marking this as ready for review to gather feedback. However, there are some limitations to acknowledge:
UPDATE: reusing the container ~23 minutes |
jglick
left a comment
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.
Not familiar with the original, but looks reasonable.
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
Is it worth keeping this test in here in this case? that seems really expensive to run... |
|
We can always decide to disable it by default in CI, while keeping the option to run it on demand to check for regressions when some are suspected. |
jtnord
left a comment
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.
GitHub won't let me post any review comments but there is at least issue of double time elasticity (don't use elastic time in a wait for) and there appear to be stream leaks (due to unclosed streams).
The idiom Jesse was looking for to ignore exceptions in a wait is .ignoring(Exception..). Using it makes the code both easier to follow and if the timeout fails and the last call resulting an exception you get told about it
I will try adding inline comments again next week, hopefully GH will have recovered.
- prefer duration - use `.ignoring` idiom to improve debugability - avoid direct elastic time usage in `Wait`
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java
Outdated
Show resolved
Hide resolved
- visit the job page to verify its existence - use logger - remove container cleanup - improve readability
src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/Dockerfile
Outdated
Show resolved
Hide resolved
Last run (reusing the container), finished in 23 minutes, with the GitLab image ready in ~4 minutes |
Attempt on optimizing the GitLab Docker image, increased timeouts, and incorporated defensive retries. Wherever possible, use Git directly instead of GitLabApi.
Thanks to @raul-arabaolaza and others for their previous work in #1425
Aim to fix:
#1461
#1365
Testing done
Submitter checklist