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

Update MSTest to metapackage and MTP #881

Merged
merged 23 commits into from
Jan 31, 2025

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jan 18, 2025

@Youssef1313 Youssef1313 requested a review from a team as a code owner January 18, 2025 08:27
test/Directory.Build.props Outdated Show resolved Hide resolved
@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

@Youssef1313 and @Evangelink, tests are failing in all environments with these changes. Please see https://dev.azure.com/mseng/1ES/_build/results?buildId=29527577&view=results for details.

@Youssef1313
Copy link
Member Author

@DaveTryon Navigating to the build shows "Build not found." for me.

@DaveTryon
Copy link
Contributor

@Youssef1313, I've emailed the logs to you and @Evangelink. They're compressed and less than 1MB, so hopefully they'll make it through your email gateway,

@Youssef1313
Copy link
Member Author

@DaveTryon I see you are adding coverage support for coverlet in #895. For MTP, Microsoft CodeCoverage is better supported. Are you okay with switching from coverlet to Microsoft CodeCov?

@Evangelink
Copy link
Member

/azp run

Copy link

Commenter does not have sufficient privileges for PR 881 in repo microsoft/sbom-tool

@DaveTryon
Copy link
Contributor

/azp run

Copy link

Pull request contains merge conflicts.

@Youssef1313
Copy link
Member Author

@DaveTryon I just fixed the conflict

@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

@DaveTryon I see you are adding coverage support for coverlet in #895. For MTP, Microsoft CodeCoverage is better supported. Are you okay with switching from coverlet to Microsoft CodeCov?

I'm not married to coverlet, but I'd like to see the MTP version before making the final call. I'll hold off on merging the PR for a bit so we can do a side-by-side comparison.

test/Directory.Build.props Outdated Show resolved Hide resolved
test/Directory.Build.props Outdated Show resolved Hide resolved
test/Directory.Packages.props Outdated Show resolved Hide resolved
@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

DaveTryon commented Jan 29, 2025

@DaveTryon it seems that E2E tests were not run before and now they are run causing the timeout. Is this expected that they run or not?

That's an interesting question. the unit tests only target net472 on Windows, but the e2e tests target net472 everywhere. The test runner, though, ignores the non-Windows net472 tests, so whether it's intentional or not, the net472 tests are effectively limited to Windows. I've sent a message to the team to see if they're OK with codifying the de facto behavior.

I did some experimenting in a branch based on your fork, and made 2 commits that you can backport to get past the test failures:

  • This commit follows the pattern from the unit tests
  • This commit gives MTP a test that succeeds, since apparently it treats it as a failure if all tests are skipped. There may also be a command line way to do this

After these commits are added, we still get an error because each stage is trying to upload to the same artifact name, and only the first one can win. Let's just go with code coverage results from Windows, since it's the platform with the broadest coverage

Copy link
Member

@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.

@Youssef1313
Copy link
Member Author

@DaveTryon Could you run the pipeline again? Thanks!

@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

@DaveTryon Could you run the pipeline again? Thanks!

@Youssef1313, it looks like the yaml is broken. The pipeline failed immediately, generating the 2 following errors but no logs:

The 'stages' parameter is not a valid StageList
/pipelines/build-test-tool-template.yaml (Line: 34, Col: 5): 'condition' is already defined

@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

@Youssef1313, the build is green again, so I think we're ready for the final clean-up phase. I'll email you the logs

@DaveTryon DaveTryon mentioned this pull request Jan 31, 2025
@DaveTryon
Copy link
Contributor

/azp run

@DaveTryon
Copy link
Contributor

DaveTryon commented Jan 31, 2025

@Youssef1313 This is super close, but now the code coverage isn't showing up in the pipeline like it used to. Happy to merge this as-is and have the code coverage be a separate PR if that makes more sense than adding it to this PR

@DaveTryon
Copy link
Contributor

/azp run

Copy link
Contributor

@DaveTryon DaveTryon left a comment

Choose a reason for hiding this comment

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

Thanks, @Youssef1313!

@DaveTryon DaveTryon merged commit 5b74448 into microsoft:main Jan 31, 2025
4 checks passed
@Youssef1313 Youssef1313 deleted the update-mstest branch January 31, 2025 17:52
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