Skip to content
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

Provide a way to change the behavior when picking up default TestAdapters #2580

Closed
Haplois opened this issue Sep 22, 2020 · 18 comments · Fixed by #3380
Closed

Provide a way to change the behavior when picking up default TestAdapters #2580

Haplois opened this issue Sep 22, 2020 · 18 comments · Fixed by #3380
Assignees

Comments

@Haplois
Copy link
Contributor

Haplois commented Sep 22, 2020

Issue

VSTest automatically picks up all the *TestAdapter.dll assemblies present with the assembly tested. We don't offer a way to change this yet. A customer reported on a feedback ticket that this delays their tests for a long time.

Proposal

Since TestAdapters and DataCollectors can be in the same assembly, specifying a different load order is not feasible. We'll use the same loading order for the assemblies, and provide a way to exclude them.

AB#1370910

@Haplois Haplois added this to the 16.9.0 milestone Sep 22, 2020
@Haplois Haplois self-assigned this Sep 22, 2020
@nohwnd
Copy link
Member

nohwnd commented Sep 22, 2020

It might be worth thinking about multiple scenarios. It would be nice if this fits well with enabling just adapters that are next to the source, and possibly allowing additional adapters to take path to the dll not just to directory. This would hopefully improve the perf as well if there are any data collected on that already @Sanan07 ?

I think flags choice for how adapters are picked up would be nice (with some nicer naming), where Default is the default:

NextToSource - pick up next to source
FromAdditionalAdapterPath - pick up in additional adapter path, but only search top level
FromAdditionalAdapterPathRecursive - pick up from additional adapter path and subdirectories
FromDefaultDirectory - pick up in default directories
Local - NextToSource, FromAdditionalAdapterPath <- maybe? to make sure the common "fast" case is easy to use
Default - NextToSource, FromAdditionalAdapterPathRecursive, FromDefaultDirectory

By also adding the ability to provide files directly to the additional test adapters, we should be able to narrow down the discovery much more than we are now.

Then we need to look at how easy it is to enable this though, and how much time it can save.

@Haplois
Copy link
Contributor Author

Haplois commented Oct 26, 2020

/cc: @jakubch1

@jakubch1
Copy link
Member

Consider having separated configs for test adapters and data collectors

@jakubch1
Copy link
Member

jakubch1 commented Oct 29, 2020

It would be good to be able to specify order. For example

<Order>Default,FromAdditionalAdapterPath,Local</Order>

@anirudhsanthiar
Copy link

anirudhsanthiar commented Oct 30, 2020

If the /TestAdapterPath:path argument is explicitly specified, then vstest should consider looking for the adapter in only that path. Currently, it picks up adapters in path as well as the folder containing the test binary. If it isn't possible to make this behavior the default, a switch to override this behavior would be good.

@pavelhorak pavelhorak modified the milestones: 16.9.0, 16.10 Mar 1, 2021
@pavelhorak pavelhorak modified the milestones: 16.10, 17.0 Jul 23, 2021
@Haplois
Copy link
Contributor Author

Haplois commented Sep 3, 2021

If the /TestAdapterPath:path argument is explicitly specified, then vstest should consider looking for the adapter in only that path. Currently, it picks up adapters in path as well as the folder containing the test binary. If it isn't possible to make this behavior the default, a switch to override this behavior would be good.

@anirudhsanthiar this behavior we'll be inside in our next preview release this Monday.

NextToSource - pick up next to source
FromAdditionalAdapterPath - pick up in additional adapter path, but only search top level
FromAdditionalAdapterPathRecursive - pick up from additional adapter path and subdirectories
FromDefaultDirectory - pick up in default directories
Local - NextToSource, FromAdditionalAdapterPath <- maybe? to make sure the common "fast" case is easy to use
Default - NextToSource, FromAdditionalAdapterPathRecursive, FromDefaultDirectory

Unfortunately this affects Test Platform very fundamentally we'll aim to provide this support in our next major release.

@Haplois Haplois modified the milestones: 17.0, Future Sep 3, 2021
@pavelhorak pavelhorak modified the milestones: Future, 17.1 Sep 21, 2021
@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Feb 3, 2022

  • We should also add a mode where we specify a specific version inside runsettings like:

lib.dll -> MD5

  • The order should be by dll in case, we can have different needs for different extensions

@nohwnd
Copy link
Member

nohwnd commented Feb 3, 2022

@MarcoRossignoli if that is the only way to specify the version then using md5 seems to be terrible for maintenance and upgrades. If that is only a mode for choosing a very specific dll and we have another mode in place that checks versions and chooses newer then I agree. But it also sound a bit unnecessary, and as an additional thing we would need to maintain together with the other mechanism that leverages versions.

@MarcoRossignoli
Copy link
Contributor

If that is only a mode for choosing a very specific dll

it's to fix "the unexpected scenario" where the order is not enough.

@nohwnd
Copy link
Member

nohwnd commented Feb 3, 2022

Ah so that would be exclude the dll with this MD5, rather than use that dll with MD5?

That was not very clear from the PR title.

@MarcoRossignoli
Copy link
Contributor

I'd say both side the chance to specify include/exclude.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Feb 3, 2022

Also discussing with @jakubch1, the idea to have another "folder" like "FirstChance", where the user put extensions and it will be check for first during the normal loading order. Always for "the unexpected scenario", to avoid the too strict MD5 strategy.

@nohwnd
Copy link
Member

nohwnd commented Feb 3, 2022

I fear that having another special place where to put stuff will become obsolete the same way the current folder became obsolete. But I am looking forward to reviewing your plan. :)

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Feb 3, 2022

will become obsolete

I like the plan of folder ordering, but I think that we need to have a workaround always fixes possible uncovered scenario.
One and only one to say "load that precise dll extension before".

@jakubch1
Copy link
Member

jakubch1 commented Feb 3, 2022

By "FirstChance" we understand a way that you add specific folder into TestAdapterPath
For example when we just added folder FirstChange like to TestAdapterPath and have now: [A,B,C,FirstChance]
then:
You are able to say that Order is for specific.dll: FromAdditionalAdapterPathRecursive[3], NextToSources, Local, FromAdditionalAdapterPathRecursive
so at the end we look in folders in order:
FirstChance, NextToSources, Local, A, B, C

For example in runsettings it could be like this:

<TestAdapterPathOrders>
      <!-- specific for 1 dll -->
      <TestAdapterPathOrder Pattern="*TraceDataCollector.dll">FromAdditionalAdapterPathRecursive[3],NextToSources,Local,FromAdditionalAdapterPathRecursive</TestAdapterPathOrder> 
      <!-- for all other dlls -->
      <TestAdapterPathOrder>NextToSources,Local,FromAdditionalAdapterPathRecursive</TestAdapterPathOrder> 
</TestAdapterPathOrders>

The issue is that azure pipelines are adding something to beginning of TestAdapterPath so it would be good to be able to specify order of usage of TestAdapterPath itself.

@MarcoRossignoli
Copy link
Contributor

cc: @Evangelink

@MarcoRossignoli
Copy link
Contributor

cc: @AbhitejJohn

@Haplois
Copy link
Contributor Author

Haplois commented Feb 16, 2022

There is an implementation for this in #3380. It doesn't currently support ordering - because order of some adapters are hard set. I allow them to be excluded, or included. A basic implementation is out with 17.1 NuGet package.

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

Successfully merging a pull request may close this issue.

6 participants