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 Generate Unit Test on Windows #1478

Merged
merged 8 commits into from
May 15, 2024
Merged

Conversation

mkondratek
Copy link
Contributor

@mkondratek mkondratek commented May 9, 2024

Fixes #1436.

Btw it fixes an issue with an error when closing a file that's content is being generated.

Test plan

  1. Generate Unit Test on both Windows and macOs

@mkondratek mkondratek self-assigned this May 9, 2024
@mkondratek mkondratek changed the title Fix uri string Fix Generate Unit Test on Windows May 9, 2024
@@ -187,8 +187,10 @@ abstract class FixupSession(
val future = group.show(range)
// Make sure the lens is visible.
ApplicationManager.getApplication().invokeLater {
val logicalPosition = LogicalPosition(range.start.line, range.start.character)
editor.scrollingModel.scrollTo(logicalPosition, ScrollType.CENTER)
if (!editor.isDisposed) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrel fix


The error appears when closing the file that contains the content being generated. This fixes it.

fun uriFor(file: VirtualFile): String {
return Rfc3986UriEncoder.encode(file.url)
private fun uriFor(file: VirtualFile): String {
val uriString = FileSystems.getDefault().getPath(file.path).toUri().toString()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks to this we have

file:///c:/Users/mikolaj/blah-blah/blah

instead of

file://c:/Users/mikolaj/blah-blah/blah

(note additional / before the disk name)

That fixes the paths in the agent. This one is related directly to the issue with:

Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 46: c:/Users/Jay Gohil/IdeaProjects/jaysVSDemo/\\c:\Users\Jay Gohil\IdeaProjects\jaysVSDemo\BubbleSortTest.java

(#1436)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Can we removed Rfc3986UriEncoder then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like we can indeed!

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but can you follow up on the following comment in Slack?

When does onOpenUntitledDocument get URIs that aren't file:? When would it be OK to take on of those and say randomly, boom, now you're a file path!?

val uri = URI.create(params.uri).withScheme("file")
if (CodyEditorUtil.createFileIfNeeded(project, uri, params.content) == null)
val uri = URI.create(params.uri)
if (!uri.path.contains("/")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth commenting why? IIUC it is because we produce VirtualFiles, and below uriFor ensures they start with /?

With that being the case, can you harden this assertion that uri.path[0] is '/'? Should we log here? Could we throw here instead?

Copy link
Contributor

@pkukielka pkukielka May 10, 2024

Choose a reason for hiding this comment

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

Or even better startsWith("/") to avoid crashes in case of empty uri string.

I would also like to see detailed comment why that code looks that way.
I suppose that might be due to a fact that we are not covering all possible file creation requests.
VSCode sometimes creates in-memory files for a temp usage (which have UUID names) and we do not support/care about those.

It's a leftover from my recent changes, perhaps we should improve it on the agent side.

if (!uri.path.contains("/")) {
return@Function false
}
val fileUri = uri.withScheme("file")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is not new, but this seems really dodgy to me. When do we get URIs that aren't file:? When would it be OK to take on of those and say randomly, boom, now you're a file path!

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact we always get a files which does not start with file:// there.
They start with untitled:// which is VSCode-specific way to say that files does not exist on the disk yet.
I should have added a comment. @mkondratek can you fix my omission and add it now?

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@mkondratek mkondratek force-pushed the mkondratek/windows-uri-fix branch 3 times, most recently from 9596cd8 to 71362f0 Compare May 14, 2024 16:15
@mkondratek mkondratek merged commit 619bf08 into main May 15, 2024
5 checks passed
@mkondratek mkondratek deleted the mkondratek/windows-uri-fix branch May 15, 2024 13:11
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.

JetBrains: Crash occurred while clicking on "Generate Unit Test".
3 participants