test: testing java method snippet arguments (related to flix/flix#8255)

[LoZander]

This adds a few tests for java method completions with placeholder snippet arguments. Related to flix/flix#8255.

[LoZander]

The production code relating to flix/flix#8255 is in the PR at flix/flix#8412

[LoZander]

Oh, shoot. I just realised my tests only test static methods. Perhaps this warrants an additional test or two? Whoever reviews this, let me know what you think :)

[magnus-madsen]

Oh, shoot. I just realised my tests only test static methods. Perhaps this warrants an additional test or two? Whoever reviews this, let me know what you think :)

It is a good place to start :)

[magnus-madsen]

In any case, I have to merge the Flix PR before this PR.

[LoZander]

I have added an additional two tests for non-static methods. These, however, fail, presumably due to the parser issues we've discussed.

[magnus-madsen]

I have added an additional two tests for non-static methods. These, however, fail, presumably due to the parser issues we've discussed.

OK. But can we keep this PR minimal then? Just with what works. Thanks!

[magnus-madsen]

What's this? Is it necessary? I don't believe we do this for other tests.

[LoZander]

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 in JavaMath, if JavaStringBuilder also exists in src, 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.

[LoZander]

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.

[LoZander]

But I suppose I could move it to the inner test suite, since it's only relevant there

[LoZander]

@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 :)

[magnus-madsen]

Maybe its OK. I am not super familiar with the codebase. Lets see what Holger says.

[magnus-madsen]

I am always paranoid about deleteFile :)

[LoZander]

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 :)

[sockmaster27]

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) running init() in setup() instead of suiteSetup() 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.

[LoZander]

Nice, good to hear. I wouldn't mind changing it to the files.test.ts approach though, if you think it's preferable :)

[magnus-madsen]

@sockmaster27 Could you give some feedback on the tests and whether they follow our architecture? Thanks!

[sockmaster27]

I just cloned this and ran it with a build of the compiler from master but the last 2 tests seem to fail. Has everything been merged on that side yet?
Sorry, missed this

I have added an additional two tests for non-static methods. These, however, fail, presumably due to the parser issues we've discussed.

[LoZander]

I just cloned this and ran it with a build of the compiler from master but the last 2 tests seem to fail. Has everything been merged on that side yet? Sorry, missed this

I have added an additional two tests for non-static methods. These, however, fail, presumably due to the parser issues we've discussed.

I referenced it loosely, but I believe this issue is why the two tests fail flix/flix#8427, to be more specific :)

[sockmaster27]

I referenced it loosely, but I believe this issue is why the two tests fail flix/flix#8427, to be more specific :)

What you can do is change test(...) to test.skip(...) for the failing tests and add a comment referencing this issue like it's done in findReferences.test.ts.

Then we'll have the pros of keeping the test and documenting the bug, while avoiding the con of having a failing test-suite.

[LoZander]

I referenced it loosely, but I believe this issue is why the two tests fail flix/flix#8427, to be more specific :)

What you can do is change test(...) to test.skip(...) for the failing tests and add a comment referencing this issue like it's done in findReferences.test.ts.

Then we'll have the pros of keeping the test and documenting the bug, while avoiding the con of having a failing test-suite.

First of all, I'm sorry for not getting back to you on this before now.

What you propose is a good idea, however since I lasted look at this, it seems the issue has been fixed. The tests pass now :)

Is there more to do in regards to this PR? Something to be changed or adjusted?

[magnus-madsen]

I will wait to merge this until we have discussed some details about what we want to test.