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.

Sign up for GitHub

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

Jump to bottom

Introduce strategies for test ID generation #1306

Merged
merged 12 commits into from
Oct 27, 2022
Merged

Introduce strategies for test ID generation #1306

merged 12 commits into from
Oct 27, 2022

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Oct 3, 2022 •
edited
Loading

Fixes #1286 and fixes AB#1634557

TODO:

  • Add comments
  • Add tests

@Haplois
Copy link
Contributor

Haplois commented Oct 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Evangelink Evangelink mentioned this pull request Oct 10, 2022
Open
@Evangelink Evangelink force-pushed the dev/haplois/id-gen branch 3 times, most recently from 6e0148c to 3ab0011 Compare October 24, 2022 16:37
Haplois
Haplois reviewed Oct 25, 2022
View reviewed changes
Copy link
Contributor

@Haplois Haplois left a comment

Choose a reason for hiding this comment

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

How is this going? Would you like any help @Evangelink?

@Evangelink
Copy link
Member Author

Hey thanks for asking @Haplois. Looks like I just need to fix CI tests (they are passing locally for me). There are still some tickets open (not fixed by this change) that I need to investigate but I feel confident we will be able to merge this one soon(TM).

@Evangelink
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Evangelink
Evangelink commented Oct 25, 2022
View reviewed changes
Copy link
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

@Haplois please feel free to do a review when you have some free time.

src/TestFramework/TestFramework/Attributes/DataSource/TestIdGenerationStrategy.cs Show resolved Hide resolved
src/TestFramework/TestFramework/Attributes/DataSource/TestIdGenerationStrategy.cs Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/Execution/TestClassInfo.cs Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/Extensions/TestCaseExtensions.cs Outdated Show resolved Hide resolved
test/E2ETests/Automation.CLI/DiscoveryEventsHandler.cs Outdated Show resolved Hide resolved
MarcoRossignoli
MarcoRossignoli previously approved these changes Oct 26, 2022
View reviewed changes
src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/ObjectModel/UnitTestElement.cs Outdated Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/ObjectModel/UnitTestElement.cs Outdated Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/PublicAPI/PublicAPI.Unshipped.txt Show resolved Hide resolved
src/Adapter/MSTest.TestAdapter/ObjectModel/UnitTestElement.cs Show resolved Hide resolved
src/Adapter/MSTestAdapter.PlatformServices/PublicAPI/PublicAPI.Unshipped.txt Show resolved Hide resolved
src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt Show resolved Hide resolved
test/E2ETests/Automation.CLI/DiscoveryEventsHandler.cs Outdated Show resolved Hide resolved
test/E2ETests/DiscoveryAndExecutionTests/TestId.DefaultStrategy.cs Outdated Show resolved Hide resolved
Haplois
Haplois previously approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@Haplois Haplois left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think removing bail-out strategy might cause issues for some users, we need to rethink that, or at lease do a version increment. My approval is grey now :(

@Evangelink
Copy link
Member Author

Thanks for the review guys!

@Haplois What do you mean by the bail-out strategy?

@Haplois
Copy link
Contributor

Haplois commented Oct 26, 2022

See my comment above on the code.

@Evangelink
Copy link
Member Author

@MarcoRossignoli @Haplois I have addressed all comments, if you want to have another look. @Haplois There is one open question remaining for you.

@Haplois
Copy link
Contributor

Haplois commented Oct 26, 2022

@Evangelink uploaded a patch file for the broken ID gen. Can't push to this branch anymore.

@Evangelink
Copy link
Member Author

@Haplois I have applied and updated your patch. Let me know if you have any other comment or if I can move forward with merging the PR.

@Evangelink Evangelink merged commit 70b1834 into main Oct 27, 2022
@Evangelink Evangelink deleted the dev/haplois/id-gen branch October 27, 2022 14:15
This was referenced Oct 27, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Haplois Haplois Haplois approved these changes

@MarcoRossignoli MarcoRossignoli MarcoRossignoli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce strategies for test ID generation
3 participants
@Evangelink @Haplois @MarcoRossignoli