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

Added support for using default credentials instead of an access token for authentication. #6

Merged
merged 4 commits into from
Jun 16, 2020
Merged

Conversation

icnocop
Copy link
Owner

@icnocop icnocop commented Jan 17, 2020

Fixes issue #3

@icnocop
Copy link
Owner Author

icnocop commented Jan 22, 2020

Do you think adding a dependency on https://www.nuget.org/packages/semver/ will be an issue?

@daveaglick
Copy link
Collaborator

I wouldn't think so - so you can parse out the current TFS version?

BTW - thanks for all the leg work on this! Let me know when it's out of draft and I'll try to quickly review and turn it around.

@icnocop
Copy link
Owner Author

icnocop commented Jan 22, 2020

Right, so the api-version string can be parsed and determine which JSON schema to use when sending requests to the TFS API.

…or authentication.

Added support to pass the API version in the logger parameters and sending the appropriate JSON schema based on the API version
Added support to optionally group test results by class name
Fixes issue #3
@icnocop icnocop changed the title [WIP] Added support for using default credentials instead of an access token for authentication. Added support for using default credentials instead of an access token for authentication. Jan 22, 2020
@icnocop
Copy link
Owner Author

icnocop commented Jan 22, 2020

@daveaglick, this is now ready for your review. :)

I've verified this version works on TFS 2015 Update 4, just like the "Publish Test Results" build task, using the following parameters:
/logger:AzurePipelines;Verbose=true;UseDefaultCredentials=True;ApiVersion=3.0-preview.2;GroupTestResultsByClassName=False

The only things I've noticed that are different so far:

  1. The name of the test run (testCaseTitle) is TestContainerName (OS: Microsoft Windows 10.0.14393 , Job: Build, Agent: AgentMachineName) for example instead of VSTest Test Run.
    Setting a more meaningful RunTitle was the reason I wanted to publish test results on the command line (during a build) instead of the "Publish Test Results" build task. ;)
  2. A trx file is not attached to each test result. This can be a feature request if/when needed.
  3. The error messages have a different format.
    They include STDERR and STDOUT and I think otherwise those are only included in the trx file.

I noticed that long error messages are cut off with a maximum of about 500 characters, but I'm not sure if the behavior is the same when using the "Publish Test Results" build task for example.

Lastly, it seems test result attachments are not uploaded, but that can be another feature request if/when needed.

Thank you.

@icnocop
Copy link
Owner Author

icnocop commented Feb 8, 2020

I went ahead and added support for uploading console outputs and errors as test result attachments and also support for uploading test result files.

@daveaglick
Copy link
Collaborator

Awesome! Hoping to get some much-needed OSS time to get this merged and deployed tomorrow )as long as nothing unexpected crops up).

@icnocop
Copy link
Owner Author

icnocop commented Feb 8, 2020

In regards to https://developercommunity.visualstudio.com/content/idea/409015/show-all-tests-in-the-hierarchy-in-test-summary.html, has it been fixed already?

The screenshots in the docs seem to indicate the test summary is calculated from all test results in all test runs even though there's a note that reads "Metrics in the test summary section, such as the total number of tests, passed, failed, or other are computed using the root level of the summarized test result.".

See the second screenshot at https://docs.microsoft.com/en-us/azure/devops/pipelines/test/review-continuous-test-results-after-build?view=azure-devops#view-summarized-test-results.

I'm not sure if the issue has been fixed and the docs just need to be updated.

For example, if you disable grouping test results by their class name with this logger, or use a classic logger and publisher, and you group the test results by their Container, does it show the expected test summary details?

@daveaglick daveaglick closed this Jun 8, 2020
@icnocop
Copy link
Owner Author

icnocop commented Jun 12, 2020

Hi @daveaglick .

I noticed this PR was closed but the code was unmerged.

Was there a reason?

Thank you.

@daveaglick
Copy link
Collaborator

Whops! Unintended consequence of changing the main branch name. I'll reopen and retarget this - I still intend to get to it. Thanks for the call out.

@daveaglick daveaglick reopened this Jun 12, 2020
@daveaglick daveaglick changed the base branch from master to main June 12, 2020 20:23
@daveaglick
Copy link
Collaborator

This is a crazy awesome PR, thanks a ton for all the legwork. I use this library in my projects, but I haven't given it much attention lately so I'm happy for the assist. Have any interest in being a co-maintainer?

ReleaseNotes.md Outdated Show resolved Hide resolved
@daveaglick
Copy link
Collaborator

For example, if you disable grouping test results by their class name with this logger, or use a classic logger and publisher, and you group the test results by their Container, does it show the expected test summary details?

Good question - I haven't been following along, but this nesting problem really annoyed me when I first started working on it. It's been a while so maybe there's hope.

@daveaglick daveaglick merged commit a49b7f8 into icnocop:main Jun 16, 2020
@icnocop
Copy link
Owner Author

icnocop commented Jun 18, 2020

Thank you Dave.

Yes, I can try to help maintain this repository if/when needed.

Thank you. 😊

@icnocop icnocop deleted the UseDefaultCredentials branch March 1, 2021 10:09
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 this pull request may close these issues.

2 participants