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

Using TestRunSystem while creating the TestRun #1637

Merged
merged 7 commits into from
Jun 28, 2018

Conversation

navin22
Copy link
Member

@navin22 navin22 commented Jun 22, 2018

  • TestRunSystem will be added as CustomTestField to test TestRunData if present.
  • Used both during merge test results true/false.
  • L0 for the same.
  • Not modifying the custom TestRunTitle if there is only one test result file to be published.
  • L0 for test run title scenarios.

navin22 added 6 commits May 30, 2018 14:14
… test case owner

- Reading the testcase attribute from junit xml as the test case owner.
- Earlier we were setting the context owner, build or the release woner as the owner of test case also.
- So removing it as that's not correct.
- TestRunSystem will be added as CustomTestField to test TestRunData if present.
- Used both during merge test results true/false.
- L0 for the same.
…t file to be published.

Added L0 for all the testruntitle scenarios.
@@ -391,5 +400,6 @@ internal static class PublishTestResultsEventProperties
public static readonly string RunTitle = "runTitle";
public static readonly string PublishRunAttachments = "publishRunAttachments";
public static readonly string ResultFiles = "resultFiles";
public static readonly string TestRunSystem = "testRunSystem";
Copy link
Member

Choose a reason for hiding this comment

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

TestRunSystem

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it would be better and consistent if I follow like other properties.

Copy link
Member

Choose a reason for hiding this comment

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

this is different from other as in this case this is predefined custom field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense. But here it's an even property which will be coming from node. So still having it as is. But when creating the field, having the right case, as you suggested.

eventProperties.TryGetValue(PublishTestResultsEventProperties.TestRunSystem, out _testRunSystem);
if (_testRunSystem == null)
{
_testRunSystem = string.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

default value should be VSTS instead of empty ? check with PM once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but that will come from the caller vsts-task-lib api call. microsoft/azure-pipelines-task-lib#373

@@ -236,7 +238,7 @@ private void ProcessPublishTestResultsCommand(IExecutionContext context, Diction
{
cancellationToken.ThrowIfCancellationRequested();
string runName = null;
if (!string.IsNullOrWhiteSpace(_runTitle))
if (!string.IsNullOrWhiteSpace(_runTitle) && resultFiles.Count > 1)
Copy link
Member

Choose a reason for hiding this comment

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

why this is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

We were adding _{count} to the custom test run title when there's more than on file. So I am just avoiding it adding when there's only one file/run. Currently it's the case for VsTest.

@TingluoHuang TingluoHuang merged commit 1ecca41 into microsoft:master Jun 28, 2018
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.

3 participants