build: Restore tests in pipeline that measures bundle size baselines

[alexvy86]

Description

https://github.com/microsoft/FluidFramework/pull/24125 recently removed the tests for this pipeline because they were broken and at the time we couldn't think of a reason why it should have been running tests in the first place. It came to light that it does it as part of our processes to get test coverage measurements, so restoring them now.

Note that some other minor changes from the original PR are not reverted since they shouldn't impact the ability of the pipeline to run tests successfully.

Reviewer Guidance

The review process is outlined on this wiki page.

[alexvy86]

Actually we can't merge this yet, we still have the issue that broke the tests before: https://dev.azure.com/fluidframework/public/_build/results?buildId=332055&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=3e8a0811-333e-5612-55d7-750fabab9520&l=450.

The tests in the bundle-size-tests package fail in CI because after the recent addition of the debug assert, in order for the tests to run successfully we first need to have run the webpack npm script for the package. The fluid-build task dependency graph for it doesn't enforce it as part of building the package, and while we have a fluid-build dependency between the tests running and webpack running first, when the pipelines run tests they don't do it through fluid-build, so webpack isn't running before them.

The actual build of the client release group doesn't fail because it has an explicit call to run webpack on every package in the repo before it runs tests. So I think the idea (discussed briefly offline) of getting rid of this pipeline entirely, and using the main build pipeline for the client release group to calculate baselines for both bundle sizes and test coverage makes more sense than continue tweaking this pipeline.

cc @jason-ha @CraigMacomber

[anthony-murphy]

should we just delete this build and enable CI for the normal PR build? is there anything that this build does that that build doesn't?

[alexvy86]

is there anything that this build does that that build doesn't?

Other than ensuring the tests run with coverage and producing artifacts for that and bundle size, I don't believe so. I'll spend some time later today to see if we need any tweaks in the main build pipeline to replicate whatever this one is doing.

[jason-ha]

@alexvy86, did you see my comment in #24125 (after merge had completed)? https://github.com/microsoft/FluidFramework/pull/24125#issuecomment-2758637662 I see two choices for bundle-size-tests concern.

1. It should say that webpack is part of it build task (or whatever task subset that is targeted).
2. Run tests under fluid-build and ensure dependencies are correct. They should already be correct for bundle-size-tests. You could almost replace - ci:test:mocha - ci:test:jest with - build-and-test:test:unit But maybe you can replace - ci:test:mocha with - build-and-test:test:mocha using this option for now.

I think I'd go with option 1. (I didn't look into details.)

All that said I would prefer coverage come from a regular CI pipeline. We don't have/do that because we use the internal form? which doesn't make the coverage results available for public PR runs to compare against? If this pipeline is for baseline bundle and coverage, then perhaps we should update naming and any descriptions (headers).

[alexvy86]

did you see my comment in https://github.com/microsoft/FluidFramework/pull/24125 (after merge had completed)? https://github.com/microsoft/FluidFramework/pull/24125#issuecomment-2758637662

I did not. I think a few threads have been mixing in my head and I'm just now untangling them: you had advocated for not making the task that runs the tests depend on the build one, which I get, but now it's also clear you weren't against the build task actually depending on the webpack one (which for some reason I thought was part of the approach you were taking in the PRs around bundle-size-tests). Yeah, having build depend on webpack makes sense to me, and then I just wonder if there was a particular reason @CraigMacomber initially made the webpack task a dependency of the test one, not the build one. Technically, it's only necessary for the tests to succeed, so I think I see a potential reasoning. But given how things work in our repo, how we run build and tests in CI, I lean towards just making webpack a dependency of the build task for the bundle-size-tests package and pay a small price in build runtime when that's all we're doing. I'll put that in a separate PR because I think we want it regardless of whether we keep the pipeline touched in this PR or get rid of it.

[anthony-murphy]

I would really bias toward simplicity here, as these types of systems tend to be a maintenance time sync. The cost of running a little more generally isn't big enough to matter when weighed against the time spent dealing with them. It takes quite a few machine hours to add up to an hour of an engineers time, and so far this seems to have taken multiple hours from multiple engineers.

we should just delete this and run the regular pipeline via ci in public.

[jason-ha]

did you see my comment in #24125 (after merge had completed)? #24125 (comment)

I did not. ... Yeah, having build depend on webpack makes sense to me, and then I just wonder if there was a particular reason @CraigMacomber initially made the webpack task a dependency of the test one, not the build one. Technically, it's only necessary for the tests to succeed, so I think I see a potential reasoning. But given how things work in our repo, how we run build and tests in CI, I lean towards just making webpack a dependency of the build task for the bundle-size-tests package and pay a small price in build runtime when that's all we're doing. I'll put that in a separate PR because I think we want it regardless of whether we keep the pipeline touched in this PR or get rid of it.

For a minimal change I would suggest "build:test": ["...", "webpack"] for the build dependency. But a more accurate change would be to redefine "build:test:esm" as having two steps:

		"build:test:esm": "npm run build:test:esm:compile && npm run webpack",
		"build:test:esm:compile": "tsc --project ./src/test/tsconfig.json",

webpack is part of the "asset" build for tests. Then you should be able to remove the "test:mocha:esm": ["...", "webpack"] override at the bottom as test:mocha:esm task already depends on build:test:esm.

What I don't know is if webpack is supported for incremental builds. That will impact any solution where it is part of build. cc @scottn12, @tylerbutler

[alexvy86]

For reference, successful run of the bundle-sizes pipeline for this PR: https://dev.azure.com/fluidframework/public/_build/results?buildId=332326&view=results. But still looking into getting rid of that pipeline instead in https://github.com/microsoft/FluidFramework/pull/24344.

[microsoft-github-policy-service[bot]]

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!