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

Making Trx Logger Hierarchical for ordered test and data driven tests #1330

Merged
merged 31 commits into from
Feb 14, 2018

Conversation

abhishekkumawat23
Copy link
Contributor

@abhishekkumawat23 abhishekkumawat23 commented Dec 8, 2017

Changed trx logger to understand data driven and ordered test in hierarchical form

If test result contains parent execution id, then results are stored in hierarchical fashion. If not, stored in flat fashion.

Parent test result is expected to comes first than child results to allow results to be hierarchical.

/// <summary>
/// Test aggregation element.
/// </summary>
internal abstract class TestAggregation : TestElement, ITestAggregation
Copy link
Contributor

Choose a reason for hiding this comment

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

TestAggregation [](start = 28, length = 15)

can we rename to TestElementAggregation..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

base.Save(element, parameters);

XmlPersistence h = new XmlPersistence();
if (testLinks.Count > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Please add parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

var parentExecutionId = Converter.GetParentExecutionId(e.Result);
var parentTestResult = GetTestResult(parentExecutionId);
var parentTestElement = (parentTestResult != null) ? GetTestElement(parentTestResult.Id.TestId) : null;
if (parentTestResult == null || parentTestElement == null || parentExecutionId == Guid.Empty)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Write a coment when we are switching to flatten way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@smadala smadala left a comment

Choose a reason for hiding this comment

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

  • Please measure perf impact.

// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move using statements to under namespace block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

private TestListCategoryId categoryId;
private List<TestEntry> testEntries = new List<TestEntry>();
Copy link
Contributor

@smadala smadala Dec 26, 2017

Choose a reason for hiding this comment

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

Create object if required? Do same in other places too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
if (parentTestElement != null &&
parentTestElement.TestType.Equals(TrxLoggerConstants.OrderedTestType) &&
!(parentTestElement as OrderedTestElement).TestLinks.ContainsKey(testElement.Id.Id))
Copy link
Contributor

Choose a reason for hiding this comment

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

When you get same testElement more than once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case orderedtest has data driven test, then we get same test element for all data driven tests. In that case we want to update test link only once.

// Create trx test element from rocksteady test case
var testElement = GetOrCreateTestElement(executionId, parentExecutionId, testType, parentTestElement, e.Result.TestCase);

// Update test links
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide test link example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

private List<TrxLoggerObjectModel.UnitTestElement> testElements;
private List<TestEntry> entries;
private Dictionary<Guid, TrxLoggerObjectModel.ITestResult> results;
private Dictionary<Guid, TrxLoggerObjectModel.ITestResult> innerResults;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@codito
Copy link
Contributor

codito commented Dec 28, 2017

Request a RFC and spec for this please. It will allow community to review the spec before the change is merged.

@abhishekkumawat23 abhishekkumawat23 changed the title [In Progress] Making Trx Logger Hierarchical for ordered test and data driven tests Making Trx Logger Hierarchical for ordered test and data driven tests Jan 3, 2018
/// <summary>
/// Uri used to uniquely identify the TRX logger.
/// </summary>
public const string ExtensionUri = "logger://Microsoft/TestPlatform/TrxLogger/v1";
Copy link
Contributor

Choose a reason for hiding this comment

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

public [](start = 8, length = 6)

nit: all these const should be near to the consumers. will be easy to read and understand


string IXmlTestStoreCustom.NamespaceUri
{
get { return null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

null [](start = 25, length = 4)

why null

{
if (id == Guid.Empty)
{
Debug.Assert(id != Guid.Empty, "id == Guid.Empty!");
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(id != Guid.Empty, "id == Guid.Empty!"); [](start = 15, length = 53)

remove assert

Copy link
Contributor

@acesiddhu acesiddhu left a comment

Choose a reason for hiding this comment

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

:shipit:

@abhishkk abhishkk merged commit d806e80 into microsoft:master Feb 14, 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.

6 participants