-
Notifications
You must be signed in to change notification settings - Fork 24
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
test: testing java method snippet arguments (related to flix/flix#8255) #440
Closed
Closed
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4f27ff9
test: completion of java method with placeholder arg
LoZander 8e28fff
test: tests for completion of java methods with placeholder snippet a…
LoZander 5f6ab5f
Merge branch 'flix:master' into test/method-arg-placeholders
LoZander 0edc81e
Merge branch 'flix:master' into test/method-arg-placeholders
LoZander bdfe919
test: testing non-static Java method calls
LoZander 99d309e
test: testing non-static Java method calls
LoZander 34d241d
Merge branch 'test/method-arg-placeholders' of github.com:LoZander/vs…
LoZander 944f812
Merge branch 'test/method-arg-placeholders' of github.com:LoZander/vs…
LoZander faef296
test: testing non-static Java method calls
LoZander 787f875
Merge branch 'test/method-arg-placeholders' of github.com:LoZander/vs…
LoZander bc03b0a
docs: docs for validCompletionExists
LoZander 44a65f3
chore: remove redundant related to Java method completion tests
LoZander 6665fbd
chore: remove redundant test code related to Java method completion t…
LoZander 5f26bbd
Merge branch 'test/method-arg-placeholders' of github.com:LoZander/vs…
LoZander e05181c
Merge branch 'flix:master' into test/method-arg-placeholders
LoZander File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
mod JavaMath { | ||
import java.lang.Math; | ||
|
||
pub def floor(x: Float64): Float64 \ IO = | ||
let r = Math.f | ||
r | ||
|
||
pub def max(x: Float64): Float64 \ IO = | ||
let r = Math.m | ||
r | ||
} |
12 changes: 12 additions & 0 deletions
12
test/testWorkspaces/completions/latent/JavaStringBuilder.flix
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
mod JavaStringBuilder { | ||
import java.lang.StringBuilder; | ||
|
||
pub def create(): StringBuilder \ IO = | ||
new StringBuilder() | ||
|
||
pub def append(s: String, builder: StringBuilder): StringBuilder \ IO = | ||
builder.a | ||
|
||
pub def toString(builder: StringBuilder): String \ IO = | ||
builder.toString() | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? Is it necessary? I don't believe we do this for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I modelled this after how it was done in the diagnostics suite. We specifically want there to only be one file in
src
at a time (even within the same suite), because the error produced by the unfinished line in one file can prevent any suggestions from appearing in the other. I at least know that some interference between tests is happening, and I assume this is it. For instance, when testing for completion suggests inJavaMath
, ifJavaStringBuilder
also exists insrc
, the test fails. I suppose that is also an error worth addressing, since the suggestions should work either way, but it feels tangential to the issue I wanted to test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, sorry, I missed that you asked what it was. I saw this done in the diagnostics suite, as I said. From context, I assume this is run at the end of each test. This way, the
src
folder is always clean upon the start of a test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I suppose I could move it to the inner test suite, since it's only relevant there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sockmaster27 proposed looking towards the diagnostics suite, so he can probably tell whether I've understood the trick used there and if I've applied it correctly here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe its OK. I am not super familiar with the codebase. Lets see what Holger says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am always paranoid about deleteFile :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, ofc. Tbh, it didn't occur to me before, but I can understand your paranoia. If it's not necessary, it's ofc. best to avoid :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfectly in line with the way it's done in diagnostics.
In
files.test.ts
I've done more or less the same thing by (admittedly inconsistently) runninginit()
insetup()
instead ofsuiteSetup()
to hard reset the contents of the entire workspace before each test. I suppose this is the more declarative way to handle it, but it's not super important to me either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, good to hear. I wouldn't mind changing it to the files.test.ts approach though, if you think it's preferable :)