Skip to content

Allow specifying default versions on ClientTestFixture#19431

Merged
pakrym merged 3 commits intoAzure:masterfrom
pakrym:pakrym/Allow-specifying-defaults-on-ClientTestFixture
Mar 11, 2021
Merged

Allow specifying default versions on ClientTestFixture#19431
pakrym merged 3 commits intoAzure:masterfrom
pakrym:pakrym/Allow-specifying-defaults-on-ClientTestFixture

Conversation

@pakrym
Copy link
Contributor

@pakrym pakrym commented Mar 10, 2021

No description provided.

@ghost ghost added the Azure.Core label Mar 10, 2021
@pakrym pakrym requested review from kasobol-msft and tg-msft March 10, 2021 19:14
@pakrym pakrym changed the title Allow specifying defaults on ClientTestFixture Allow specifying default versions on ClientTestFixture Mar 10, 2021
Copy link
Contributor

@kasobol-msft kasobol-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

@pakrym pakrym marked this pull request as ready for review March 11, 2021 20:02
@pakrym pakrym enabled auto-merge (squash) March 11, 2021 20:48
@pakrym pakrym merged commit 74607f8 into Azure:master Mar 11, 2021
@weshaggard
Copy link
Member

cc @benbp
@pakrym do you you have thoughts on how we would setup which service version we want to run the tests against?

@pakrym
Copy link
Contributor Author

pakrym commented Mar 15, 2021

Adding an environment variable (or something that flows into the .env file) with a list of supported service versions would be an easy solution.

We would then intersect that with the list we have in code and run only supported tests.

@benbp
Copy link
Member

benbp commented Mar 15, 2021

@maorleger and I were just talking about a pattern along the same lines for js. I think we can pass the service version in via the matrix, and plumb through the env variables in the yaml.

@weshaggard
Copy link
Member

Wouldn't we also need to use that information to setup the clients we are creating in our tests? By default the clients usually use the latest api version so we would need to either pass in a version or maybe start to have a pattern where our client libraries could read some well known env variable as the default api version when being constructed.

@pakrym
Copy link
Contributor Author

pakrym commented Mar 16, 2021

@maorleger
Copy link
Member

maorleger commented Mar 17, 2021

@pakrym I'm doing something similar for JS, maybe we can sync up offline to see if we can align on the approach?

Do you have any concerns about testing every service version across every matrix / build configuration (as far as it adds to the run time)? One thing I was considering is having every live test run against the latest service version plus one live test that runs against every service version. So our environment variable might be more along the lines of "enable/disable multi-version testing" and library authors would need to opt-in by reading that variable and setting the service versions accordingly.

We might have something like:

  • Node 14 Windows (latest service version)
  • Node 10 Windows (latest service version)
  • ....
  • Node 14 OS X (latest service version)
  • ...
  • (One additional) Node 14 Windows (every service version)

What do you think about that?

@pakrym
Copy link
Contributor Author

pakrym commented Mar 17, 2021

maybe we can sync up offline to see if we can align on the approach?

Sure.

Do you have any concerns about testing every service version across every matrix / build configuration (as far as it adds to the run time)?

Yeah, testing all service versions an all operation systems seems excessive. We already have a flag to only run the latest service version and are considering adding an additional flag to specify which exact service version to run. But I also think it's useful to be able to run a set of tests against all versions in development.

We are trying to build up additional features when requirements come up and see how they work out.

What do you think about that?

I think it's reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants