Could you explain your overall plan for the refactoring? realWin wraps them in friendly iframe, which means those tests can still access global document. So this single PR does not change that behavior. Also, some tests might choose describes.fakeWin or describes.sandbox depends on their own need.

Could you explain your overall plan for the refactoring? realWin wraps them in friendly iframe, which means those tests can still access global document. So this single PR does not change that behavior.

You're correct. The next step is to add a mechanism to describes to detect when a test touches global state, and either prevent it or clean it up.

Also, some tests might choose describes.fakeWin or describes.sandbox depends on their own need.

I'm going to leave that decision to test authors. Right now, all we're doing is converting legacy describe tests to use describes.realWin (for unit tests) or describes.integration (for integration tests). I've added a presubmit check to this PR so that future tests written with legacy describe will be flagged as an error, with a suggestion to use one of the describes types.

@lannka @jridgewell @choumx This PR is now ready for a full review.

The next step is to add a mechanism to describes to detect when a test touches global state, and either prevent it or clean it up.

Any idea how would you do that?

I've added a presubmit check to this PR so that future tests written with legacy describe will be flagged as an error, with a suggestion to use one of the describes types.

This sounds a right thing to do.

The next step is to add a mechanism to describes to detect when a test touches global state, and either prevent it or clean it up.

Any idea how would you do that?

I believe this can be done in setup() and teardown() in RealWinFixture and IntegrationFixture in testing/describes.js. I'll have to do some experimentation to see what state is currently being changed by tests.

I believe this can be done in setup() and teardown() in RealWinFixture and IntegrationFixture in testing/describes.js. I'll have to do some experimentation to see what state is currently being changed by tests.

It might be possible to detect those calls, but I don't see a straightforward way to automatically fix all those global access. That said, we will likely end up fixing tests one by one. If that's the case, I don't see too much value of wrapping those tests in realWin, which in fact makes the tests code a bit more unreadable.

To force all new tests to use describes, I guess you can create a whitelist of those legacy tests, and have the team to fix them one by one?

I don't see too much value of wrapping those tests in realWin, which in fact makes the tests code a bit more unreadable.

Fair enough. I'll revert the test changes.

To force all new tests to use describes, I guess you can create a whitelist of those legacy tests, and have the team to fix them one by one?

Can do. I'll rework this PR to be a presubmit check instead, with a whitelist containing all the files that currently use describe.

It's theoretically possible to force anyone who touches a test file to also change describe to describes.*. But this will become annoying during large refactors. (We did this in the past for JSDoc errors, and it didn't go down well.)

@lannka Changed this to a presubmit check and whitelisted all existing uses. PTAL.