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

Better "Tests" detection #800

Closed
TheAngryByrd opened this issue Mar 1, 2023 · 10 comments · Fixed by #801
Closed

Better "Tests" detection #800

TheAngryByrd opened this issue Mar 1, 2023 · 10 comments · Fixed by #801

Comments

@TheAngryByrd
Copy link

👋 I was trying out fsdocs in repository. When trying to generate docs for reference information I kept getting:

  skipping project 'Project1.fsproj' because it looks like a test project

Turns out it's because I'm in a directory that has the word "test" in it. "fsdocstester/src/Project1".

if s.Contains(".Tests") || s.Contains("test") then

I know it's an unlikely case but I'm always the one to hit those 😂 .

@nojaf
Copy link
Collaborator

nojaf commented Mar 1, 2023

Hi there, thanks for bringing this up.
What would you suggest here? Something more along the lines of EndsWith?

@nhirschey
Copy link
Collaborator

nhirschey commented Mar 1, 2023

Another option is don't filter out test projects if they're specified by the --projects option. That would allow fsdocs build --projects "fsdocstester/src/Project1.fsproj"

I believe we'd get that functionality if we moved the "test" project filtering so that it only applies inside match case on line 392.

@nojaf
Copy link
Collaborator

nojaf commented Mar 1, 2023

Good suggestion!

@TheAngryByrd
Copy link
Author

Hi there, thanks for bringing this up. What would you suggest here? Something more along the lines of EndsWith?

I'm guessing this code is trying to avoid cracking as we could look post crack at either IsTestProject or if Microsoft.NET.Test.Sdk is referenced.

@baronfel
Copy link
Collaborator

baronfel commented Mar 1, 2023

Honestly, just lean into cracking :) If you use something like buildalyzer to 'drive' the build out of proc it should be relatively stable across MSBuild versions, and this property only relies on evaluation not execution so changes in target ordering across SDK versions shouldn't impact you.

From a performance perspective, you can toggle MSBuild Server on (or wait for it to be toggled on by default) and a lot of the process-spawning overhead should just disappear.

@nojaf
Copy link
Collaborator

nojaf commented Mar 1, 2023

This code might actually pre-date Microsoft.NET.Test.Sdk 😅.
So if there is a better way to detect this I'm all ears.

@baronfel
Copy link
Collaborator

baronfel commented Mar 1, 2023

<IsTestProject>true</IsTestProject> is the canonical way to detect this. IMO we should use it. The Microsoft.NET.Test.Sdk contributes setting this property by default, but it's also possible for projects to add it manually.

@nojaf
Copy link
Collaborator

nojaf commented Mar 1, 2023

That sounds good to me. How would one achieve this easily? Loading the fsproj file, running some target and reading the property? Something along those lines?

@baronfel
Copy link
Collaborator

baronfel commented Mar 1, 2023

You shouldn't need to run any targets - the act of loading the project file is evaluation, and after evaluation you have access to all properties that are statically-known (including those delivered by SDKs). Evaluation is generally quite fast, it's running design-time-builds (like editors need to to get compiler options) that takes some time.

@nhirschey
Copy link
Collaborator

How would one achieve this easily?

I think we just delete lines 426-433. projectFiles is used to generate projectInfos, and projectInfos is already filtering out test files using this property

printfn " skipping project '%s' because it has <IsTestProject> true" shortName

We'd need to test but scanning the code, just delete 426-433.

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

Successfully merging a pull request may close this issue.

4 participants