Added CI/Git information to test spans for CI providers #1940
Conversation
drodriguezhdez
added
the
tag: no release notes
drodriguezhdez
force-pushed
the
drodriguezhdez/ci_providers
branch
from
5955fce to
d45d49d
Compare
| setJenkinsData(); | ||
| } else if (System.getenv(GITLAB) != null) { | ||
| setGitLabData(); | ||
| } else if (System.getenv(TRAVIS) != null) { |
There was a problem hiding this comment.
Do you want a hierarchy here instead?
There was a problem hiding this comment.
I think it's fine to have it like this. A hierarchy for CI/Git decorators is too specific for the "CiApp" use case, and it will not be reusable for any other case. I'd not create a lot of decorators only for this case, IMHO.
There was a problem hiding this comment.
I think the code would be easier to read if things were pulled apart a bit, so that you have one abstract CiInformation class that has all the methods that you need to implement, i.e. getProviderName(), getPipelineId et.c., and then have a specific subclass for each provider like gitlab, travis, et.c. As it's written right now, I can't really see if all the setter methods actually set all the fields they should.
There was a problem hiding this comment.
Refactored into different CIProviderInfo implementations.
| import org.junit.contrib.java.lang.system.EnvironmentVariables | ||
| import spock.lang.Shared | ||
|
|
||
| import static datadog.trace.bootstrap.instrumentation.decorator.TestDecorator.APPVEYOR |
There was a problem hiding this comment.
Could this be a wildcard import?
There was a problem hiding this comment.
I think we could do it, but setting a smaller number in the Imports configuration in the IDE.
Now, following the CONTRIBUTING.md, is set to 9999.
* Editor > Code Style > Java/Groovy > Imports
* Class count to use import with '*': `9999` (some number sufficiently large that is unlikely to matter)
* Names count to use static import with '*': `9999`
There was a problem hiding this comment.
As TestDecoratorTest has been refactored, this comment has been resolved.
Added CI/Git information to test spans for the following CI providers: