Skip to content

Make tests faster & better (use completions API) #103

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

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jan 25, 2023

I tested on singleline tests only: on my macbook m1 its reduced time 58s -> 8s

Would like to receieve feedback on it!

I decided to use API until microsoft/vscode#166094 is resolved, because, awaiting workspace.applyEdit works properly.

@zardoy
Copy link
Contributor Author

zardoy commented Jan 25, 2023

Also I think there is something wrong with end of lines were in changed files. Currenty end_of_line in editorconfig is set to crlf (I don't remember why) maybe it should be set to unset

@ipatalas
Copy link
Owner

I'll definitely take a look on that as this is a pain in the ass to wait for those tests so long.

@zardoy
Copy link
Contributor Author

zardoy commented Jan 25, 2023

Intention of this PR is to eliminate delay calls and make things as fast as possible, I will probably include other test improvements here. First part seems okay as I can see on ci.

@zardoy zardoy changed the title use testing via API for simple completions (without commands) Make tests faster & better (use completions API) Jan 25, 2023
@zardoy zardoy mentioned this pull request Jan 25, 2023
2 tasks
@@ -2,7 +2,7 @@ import * as vsc from 'vscode'
import * as ts from 'typescript'
import { invertExpression } from './utils/invert-expression'

export const NOT_COMMAND = 'complete.notTemplate'
export const NOT_COMMAND = '_postfix.applyNotTemplate'
Copy link
Owner

Choose a reason for hiding this comment

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

To be honest I have an idea to drop this command at all and replace this with multiple versions of the template, so instead of picking not template and then selecting from quick pick one would see not#1, not#2 (or sth similar)

This would not only help with those tests here but also make it more friendly to the user in my opinion + less clicks to apply the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea. While it can make sense in case when I need to apply not on identifier (identifier not will always go first and it takes less clicks), but when I need to always revert whole binary expression it might be painful for users that have:

  1. Other templates starting with not eg notZ will be last
  2. For js only: word-suggestions

Screenshot 2023-02-02 at 01 58 45

It will be hard to apply last variant, so I don't see any advantages of it (for example web storm also provides selector as second step).

But we definitely need to add at selector (0 or -1) support for not template. So I can have postfixes that always revert either identifier or whole binary epxression.

BTW I renamed it here so its not shown in keybindings.json command autocomplte and other locations as commands starting with _ are internal.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, for complex binary expressions that could be tricky, but who does more nesting that just several levels on the other hand? :)
Anyway even if I decide to pursue this way this should probably be optional as some people might prefer current way.

@zardoy
Copy link
Contributor Author

zardoy commented Feb 1, 2023

thx to github, got notification of commit above. I don't think we need to bump engine until actually new API is used within extension (it is used outside, in our tests)

@ipatalas
Copy link
Owner

ipatalas commented Feb 1, 2023

I don't think we need to bump engine until actually new API is used within extension (it is used outside, in our tests)

True that but maybe it's time, I haven't bumped it for a while and I'm pretty sure 95%+ users are updating on regular basis so this should be just fine especially that 1.72 is from September 2022.

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.

2 participants