Adding multitarget to integration tests

[eddynaka]

Fixes #.

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1266 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1266      +/-   ##
==========================================
- Coverage   79.14%   79.11%   -0.04%     
==========================================
  Files         215      215              
  Lines        6176     6176              
==========================================
- Hits         4888     4886       -2     
- Misses       1288     1290       +2     

Impacted Files	Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	77.77% <0.00%> (-3.18%)	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	97.14% <0.00%> (-2.86%)	⬇️
...us/Implementation/PrometheusExporterEventSource.cs	72.72% <0.00%> (+9.09%)	⬆️

[xiang17]

Hey, I created an issue for this #1264 You've beat me in fixing it 😄

[CodeBlanch]

@eddynaka I was just doing a bit of googling. It looks like Docker supports variable usage in the FROM. What if we did something like?

ARG BASE_IMAGE_TAG=3.1
FROM mcr.microsoft.com/dotnet/core/sdk:${BASE_IMAGE_TAG} AS final

Idea being in the compose you could set BASE_IMAGE_TAG=2.1. Might remove the need to go and download 2.1 and install it into the 3.1 image. 🤷

[eddynaka]

So, let's separate in two cases:

• netcoreapp3.1: we don't need to do anything
• netcoreapp2.1: we must have sdk 2.1 and 3.1 installed

In the end, we would have to add the installation either way, no?

[CodeBlanch]

netcoreapp2.1: we must have sdk 2.1 and 3.1 installed

If that's the case, ya it won't do anything to help. Just curious, why do we need both 2.1 & 3.1 in there to run 2.1 tests?

[eddynaka]

To compile, we need the highest framework, so dotnet 3.1.
To run, we need that framework.

When I was testing, I moved to 2.1 only and I couldn't compile.
When I just tried to publish 2.1 (3.1 image), we can do that, but when we test, it was failing because it wasn't finding the runtime.

[CodeBlanch]

@eddynaka I was able to get it work for Redis but I had to make some tweaks. Check out: CodeBlanch@074da8a

1. Test project file change. This is the main thing. It seems like even if you ask for 2.1, if netcoreapp3.1 is in csproj, you need 3.1 SDK to compile. So what I needed to do was allow the TargetFrameworks list to be overriden at build/publish time. This way we can specify only 2.1, which works on 2.1 SDK.

    <TargetFrameworks Condition="$(TARGET_FRAMEWORK) == ''">netcoreapp2.1;netcoreapp3.1</TargetFrameworks>
    <TargetFrameworks Condition="$(TARGET_FRAMEWORK) == '' and $(OS) == 'Windows_NT'">$(TargetFrameworks);net461</TargetFrameworks>
    <TargetFrameworks Condition="$(TARGET_FRAMEWORK) != ''">$(TARGET_FRAMEWORK)</TargetFrameworks>

2. Dockerfile change. I added the concept of SDK_TAG into the Redis dockerfile. Used on both FROM lines. Also sets the new TARGET_FRAMEWORK flag to PUBLISH_FRAMEWORK argument. That's hooking up to the magic introduced in step 1.

ARG SDK_TAG=3.1
FROM mcr.microsoft.com/dotnet/core/sdk:${SDK_TAG} AS build
ARG PUBLISH_CONFIGURATION=Release
ARG PUBLISH_FRAMEWORK=netcoreapp3.1
WORKDIR /repo
COPY . ./
WORKDIR "/repo/test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests"
RUN dotnet publish "OpenTelemetry.Instrumentation.StackExchangeRedis.Tests.csproj" -c "${PUBLISH_CONFIGURATION}" -f "${PUBLISH_FRAMEWORK}" -o /drop -p:IntegrationBuild=true -p:TARGET_FRAMEWORK=${PUBLISH_FRAMEWORK}

ARG SDK_TAG
FROM mcr.microsoft.com/dotnet/core/sdk:${SDK_TAG} AS final
WORKDIR /test
COPY --from=build /drop .
ENTRYPOINT ["dotnet", "test", "OpenTelemetry.Instrumentation.StackExchangeRedis.Tests.dll"]

3. Call docker build and specify PUBLISH_FRAMEWORK & SDK_TAG. Compose file updated to do that, but you can also call on the command line like this:

docker build --file test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests/dockerfile . --build-arg SDK_TAG=2.1 --build-arg PUBLISH_FRAMEWORK=netcoreapp2.1

Seems to work. Is it better than installing 2.1 into a 3.1 image? I think so, but not sure if everyone will agree 😄

Pros:

• More of a real test. Build & test using 2.1 SDK. 3.1 not in the picture at all.
• This will be easier to support .net 5, .3.1, & 2.1 in the future I think. Only need to add a new compose with SDK_TAG=5.0 PUBLISH_FRAMEWORK=net5.0 and it should be good to go.

Cons:

• Project file voodoo.

[CodeBlanch]

PS: If you decide to run with what I did on that branch, follow these steps:

• Add the csproj tweaks for Redis & SqlClient test projects.
• Update the docker files for Redis & SqlClient to use SDK_TAG & pass -p:TARGET_FRAMEWORK=${PUBLISH_FRAMEWORK}.
• Create a compose file for each target framework like you are doing on this PR. Set the args in those compose files for sdk & publish:

      args:
        PUBLISH_FRAMEWORK: netcoreapp2.1
        SDK_TAG: 2.1

No idea how the W3C tests work you are on your own for those 👍

[eddynaka]

OMG that is complex. i think we can invert and install 3.1 in 2.1 image. but, doing that change to csproj I don't think will be easy to understand/maintain.

[eddynaka]

after the last change that I made (inverting from 3.1 to 2.1 as base image and installing 3.1 when needed or starting at 3.1) i'm seeing some issues when running 2.1 image. Can you see what am i doing wrong?

[CodeBlanch]

Hah! I've seen worse 😄 Check out DiagnosticSource csproj. Let me take a look.

[CodeBlanch]

Update. I think everything is working now. I went with the csproj method. Seems simpler to me! @cijothomas @alanwest @reyang anyone feel passionate one way or the other?

What changed? It seems like with the 2.1 SDK you can't dotnet test a DLL. You have to give it a csproj. Doing dotnet vstest with a DLL seems to work fine on both 2.1 & 3.1, so I used that for these integration tests.

[eddynaka]

@CodeBlanch , any reason why we need to add this? We are receiving some warnings after this change.

[CodeBlanch]

Dang. I had to add that to get the W3C tests to pass on 2.1. Without the version they complain about no lower bound version or something? If you take this out in the 2 spots and then run the 2.1 docker-compose for W3C you should see the error.

[eddynaka]

https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/260622003

[CodeBlanch]

Updated

[eddynaka]

i saw ur hack! 👍

[CodeBlanch]

LGTM