Improve Assert APIs

[CraigMacomber]

Description

Enables debugAsserts by default in development configurations, but adds a more robust option for testing with them (and similar development only) code disabled.

Allows passing debug/development only formatted text into asserts via callbacks. This change makes it possible to have descriptive assert messages with string interpolation for debug/development builds, but still get the benefits of minimized tagged asserts in production.

Breaking Changes

The format for assertion error messages in builds that do not enable production bundle optimizations can now include a second line with additional text.

This could presumably break some things, but it is opt in and has no impact on existing asserts.

Reviewer Guidance

The review process is outlined on this wiki page.

[CraigMacomber]

Still useful.

[anthony-murphy]

I think the testing story really needs to be addressed before we expand usage here. Real bugs can happen when test code and prod code diverge, which this seems to increase the capability for. We should get a plan in place to address the testing gap with the current debug asserts before we move forward adding more capabilities for debug only code. Unfortunately, fixing the testing problem will likely be quite costly in terms of both building and maintaining the capability to test all our code under both debug and prod variants.

[CraigMacomber]

I think the testing story really needs to be addressed before we expand usage here. Real bugs can happen when test code and prod code diverge, which this seems to increase the capability for. We should get a plan in place to address the testing gap with the current debug asserts before we move forward adding more capabilities for debug only code. Unfortunately, fixing the testing problem will likely be quite costly in terms of both building and maintaining the capability to test all our code under both debug and prod variants.

I think there is a possibly larger is a possibly larger risk related to not including validation which we deem too expensive for execution in prod. Currently not testing which such validation has already caused at least one bug (in a test in this case) which this change allowed mt to discover and fix.

Given the pressure for small bundle size adding additional validation of internal invariants in debug asserts seems quite useful, but having them disabled in tests makes it mostly pointless.

Perhaps we could recommend that no one make any debug only code that could have any risk of sideffects unless the explicitly unit test with and without it manually?

This seems better than just not adding and extra validation out of fear of bundle size and perf regressions.

There are risks either way, and I'd like to empower developers to decide when development only validation is a new win, rather than not providing the option at all.

Perhaps making our fuzz tests randomize the value of emulateProductionMode, or ensuring our end to end and/or jest tests use a full production bundle would be enough of a safety new to justify this?

[anthony-murphy]

To be honest I'm not totally clear on how it all works, but maybe individual packages could test both somehow? I think in most our package.json's we have targets for both cjs and esm, could we do that same for this somehow? we don't actually run the cjs one. This is just an example idea. But i do think we need some testing pattern to validate both, even if it is somewhat manual.

[CraigMacomber]

To be honest I'm not totally clear on how it all works, but maybe individual packages could test both somehow? I think in most our package.json's we have targets for both cjs and esm, could we do that same for this somehow? we don't actually run the cjs one. This is just an example idea. But i do think we need some testing pattern to validate both, even if it is somewhat manual.

This PR includes tests for both for some selected test suites which test code making heavy use of debugAssert. See https://github.com/microsoft/FluidFramework/pull/24013/files#diff-ea23da421dfe6f3b883e8a2b23b37a607c7a01c56cb9eb4772af5d956535abf2 which demonstrates a pattern which can be used to do this on the test suite level.

This same pattern can be done equally well with a cli flag or environment variable and a top level hook for a whole package if desired (similar to how we handle performance testing). I have now configured the tree package to provide a package.json script test:mocha:prod which does this, and confirmed its passing. This is not run by CI (just like the CJS tests).

[CraigMacomber]

Still useful.

[CraigMacomber]

Follow-up idea: maybe we could use a policy handler that ensures (or tries its best to ensure) that test:mocha invokes test:mocha:prod if a package uses debugAssert anywhere, so it's harder for us to forget to test with them disabled.

I like the idea, though it seems a bit complex to implement. If debugAssert gets wider adoption, that could become worth doing.

[github-actions[bot]]

🔗 Found some broken links! 💔

Run a link check locally to find them. See https://github.com/microsoft/FluidFramework/wiki/Checking-for-broken-links-in-the-documentation for more information.

linkcheck output

> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

 ELIFECYCLE  Command failed with exit code 1.

[Josmithr]

This PR includes examples of three approaches for catching such issues:

Cases where debugAssert is more heavily used can have test suites which explicitly disable it (or loop over both modes). Since debugAssert is often most useful in performance sensitive code, doing this for the performance test suite is a good option. > Example in jsonCursor.bench.ts A package level script can be added to run the all the tests in prod mode, see test:mocha:prod The above script can be fully or partly included in the CI tests, see test:mocha which runs the smoke test subset.

My 2 cents: If we're going to predicate anything on environment "modes", we should just run our entire test suite(s) in all applicable modes (presumably "dev" and "prod"). Not something we need to do in this PR, but something we should commit to and get scheduled before merging this PR.

[CraigMacomber]

This PR includes examples of three approaches for catching such issues:

Cases where debugAssert is more heavily used can have test suites which explicitly disable it (or loop over both modes). Since debugAssert is often most useful in performance sensitive code, doing this for the performance test suite is a good option. > Example in jsonCursor.bench.ts A package level script can be added to run the all the tests in prod mode, see test:mocha:prod The above script can be fully or partly included in the CI tests, see test:mocha which runs the smoke test subset.

My 2 cents: If we're going to predicate anything on environment "modes", we should just run our entire test suite(s) in all applicable modes (presumably "dev" and "prod"). Not something we need to do in this PR, but something we should commit to and get scheduled before merging this PR.

By that argument, we should be running our full test suite on bundled code in the browser and on ESM builds as we have both ESM specific stuff and browser specific stuff, and the bundled code is clearly different and can break stuff as well.

Running our full test suite on all our configurations is something we explicitly did work to avoid doing in the ESM case, and have never even tried to do in the bundler/browser case. If we ever actually do a bundled + browser test run, then I'd recommend that do a production configured bundle to actually test how end users will use our code, but I don't think we want to block having extra debug only validation on having a plan to commit to doing that.

Lack of ability to do debug only validation has real costs (correctness, dev time, bundle size, and performance) and I'd like our developers to be able to make the tradeoff decision of using debugAsserts vs nothing vs assert even if we don't have a plan to have a full test run with debugAsserts removed. I don't think we have the dev or CI budget or desire to do such testing, and I don't want people making the choice to use debugAssert assuming a plan which likely will not get funded will happen.

For years I have wanted us to run performance tests on our optimized bundled builds. We never managed to fund that and I simply don't see us wanting to do similar work related to debugAsserts.

If debugAsserts are not an ok thing to have without a plan to drastically increase our CI time to test everything with and without them, then I don't think we should land this as we have no plan, and if we had such a plan, I don't think doing it would be desirable or funded.

In this PR I have set things up such that any testing of production mode code for code using debugAssert can be done if desired on a case by case basis in multiple different ways with examples of each, including running them on CI and/or manually. Authors of code using debugAssert can choose how/if they want to test it. I think that should be enough.

[Josmithr]

By that argument, we should be running our full test suite on bundled code in the browser and on ESM builds as we have both ESM specific stuff and browser specific stuff, and the bundled code is clearly different and can break stuff as well.

Yeah, I would agree with this. It really doesn't seem like this should be that difficult. There is a cost associated with it, but each "flavor" of test run could be done in parallel and would be of significant long-term benefit to the team. Obviously, this is outside the scope of this PR.

Running our full test suite on all our configurations is something we explicitly did work to avoid doing in the ESM case, and have never even tried to do in the bundler/browser case. If we ever actually do a bundled + browser test run, then I'd recommend that do a production configured bundle to actually test how end users will use our code, but I don't think we want to block having extra debug only validation on having a plan to commit to doing that.

Team Rohan was investigating browser environment testing. @chentong7 drove this, and I believe the intention was for the testing crew to follow up (@alexvy86 to confirm).

I agree that we shouldn't block this work on having our desired testing matrix setup, but I would very much like to see us start making steps in that direction. Your changes don't make our current testing situation worse so much as they further expose gaps in that testing story. I worry that devs will not write "prod-like mode" tests, even if they should. And I worry that we will continue to add more "dev" vs "prod" logic over time without getting our house in order first.

But to be clear: I do think this PR is a net benefit. I've approved, and I'm happy to see it go in as it is. But I'm also hoping we can push to get a more robust testing story in place.