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

fix: show at most one decoration per line #598

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Jan 17, 2025

Follow-up to #590.

Instead of capping the number of decorations per file, we now show at most one decoration per line. This still places a good upper limit so we don't overwhelm VS Code rendering with too many decorations. But it also prevents the confusing UI of decorations abruptly cutting off.

Screen.Recording.2025-01-17.at.10.32.07.mov

@Skn0tt Skn0tt requested a review from pavelfeldman January 17, 2025 09:35
@Skn0tt Skn0tt self-assigned this Jan 17, 2025
Copy link
Member

@pavelfeldman pavelfeldman left a comment

Choose a reason for hiding this comment

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

A test would be nice!

src/extension.ts Outdated
@@ -675,25 +675,23 @@ export class Extension implements RunHooks {
activeDecorations.push({ range: location.range });
}

const completedDecorations: vscodeTypes.DecorationOptions[] = [];
const completedDecorations: Record<number, vscodeTypes.DecorationOptions> = {};
for (const { location, duration } of completed) {
if (uriToPath(location.uri) === uriToPath(editor.document.uri)) {
Copy link
Member

Choose a reason for hiding this comment

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

drive-by I would pre-compute the editor's uri instead of doing it 1000 times. Maybe even store decorations by line from the get go.

@Skn0tt
Copy link
Member Author

Skn0tt commented Jan 20, 2025

A test would be nice!

the previous PR already added a good test:

test('should limit highlights', async ({ activate }) => {
const { vscode, testController } = await activate({
'playwright.config.js': `module.exports = { testDir: 'tests' }`,
'tests/test.spec.ts': `
import { test, expect } from '@playwright/test';
test('pass', async () => {
for (let i = 0; i < 2000; i++) {
expect(i).toBe(i);
}
});
`,
});
await vscode.openEditors('**/test.spec.ts');
await new Promise(f => testController.onDidChangeTestItem(f));
await testController.run();
const decorationsLog = vscode.window.activeTextEditor.renderDecorations(' ');
const lastState = decorationsLog.substring(decorationsLog.lastIndexOf('------'));
expect(lastState.split('\n').length).toBeLessThan(2000);
});

@pavelfeldman pavelfeldman merged commit 1f77db1 into microsoft:main Jan 28, 2025
4 of 6 checks passed
@toduyemi
Copy link

toduyemi commented Feb 5, 2025

Is there a way to config this? These decorations were something i relied on heavily in my debug flow, like to get a sense of how many iterations a loop took, which logic branches were seeing heavy traffic in a loop etc. This was not something that needed to be "fixed". :(

@Skn0tt
Copy link
Member Author

Skn0tt commented Feb 6, 2025

That's good feedback! This fix is mostly for performance reasons. We need to limit the decorations in some way, otherwise VSCode gets overloaded. Let's think about what a middle ground would be where there's no performance problems, but your debug flow also isn't affected.

When you debug your tests that way, do you run a single test, or do you run all tests in file? We could try only engaging this optimisation if there's more than one test being run.
Or we could limit the decorations to 5 per line instead of 1 per line (or any other low number).

Let me know what you think would be a good solution!

@toduyemi
Copy link

toduyemi commented Feb 6, 2025

Gratitude for the swift reply!

In situations where I'm paying attention to the decorations, I'm almost definitely running a single test. I generally set most loops to 10.

We could try only engaging this optimisation if there's more than one test being run.

This seems like a great middle ground, i'd be happy with. Another potential solution would be including some sort of counter in the decoration alongside the elapsed time. So it would tell you that {the 5th execution of this line of code took 400ms} or something to that effect.

The decorators really have proven very useful while authoring a test or light debugging (means I don't have to launch debugger or PW Inspector). And since the decorations persist after the test is done, I can still refer to them as tinker with the app or test script. So i really appreciate taking this into consideration :) also love the other updates in this release. Particularly the playwright drawer!

@Skn0tt
Copy link
Member Author

Skn0tt commented Feb 7, 2025

Thanks! I like the counter and will open a PR for it. We like having issues for each change we ship - could you open this as a feature request over in the playwright repo? Thank you 🙏

@toduyemi
Copy link

Will do! Gratitude :)

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