-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for running tests #144
Conversation
plugin/src/main/java/com/microsoft/java/bs/gradle/plugin/SourceSetsModelBuilder.java
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Outdated
Show resolved
Hide resolved
@jdneo Added |
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 will next try to integrate this into the java test runner extension and see if there is any gaps:
test runner delegates test request to GBS -> GBS reports the test result back -> test runner shows the test result.
This PR will be suspended temporarily during this progress. Thank you @Arthurm1 for your contribution!
server/src/main/java/com/microsoft/java/bs/core/internal/gradle/GradleApiConnector.java
Outdated
Show resolved
Hide resolved
@jdneo I've raised this build-server-protocol/build-server-protocol#674 I've gone for new data types as I don't think |
server/src/main/java/com/microsoft/java/bs/core/internal/services/BuildTargetService.java
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Outdated
Show resolved
Hide resolved
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.
Thank you @Arthurm1, I'll continue the integration work between this work and the java test runner extension.
BTW, I guess we need to update this proposal: build-server-protocol/build-server-protocol#674 as well?
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Outdated
Show resolved
Hide resolved
I'll update it when we're completely happy with the structure here. |
@@ -370,6 +467,376 @@ void testCompilingSingleProjectServer() { | |||
}); | |||
} | |||
|
|||
@Test | |||
void testRunTestsProjectServer() { |
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 we can consider to separate the test feature tests to a separate file since this is a big one. And I have a feeling that more and more cases will be added for different test framework, and different usage for the same framework.
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.
Happy to move the testing to a different integ test.
server/src/main/java/com/microsoft/java/bs/core/internal/services/BuildTargetService.java
Outdated
Show resolved
Hide resolved
BTW, during the integration, I found an issue about the JUnit 5 @nested support for Gradle Test Launcher: gradle/gradle#29603 Just in case you may have the same problem. |
server/src/main/java/com/microsoft/java/bs/core/internal/reporter/TestReportReporter.java
Outdated
Show resolved
Hide resolved
Added |
Some test cases failed. Except for that, the PR LGTM. One last thing I hope to do is to separate the related test cases to another file. I can do that as well. @Arthurm1 Let me know if you want me to do that. |
@jdneo I can't get the test classes to fail locally. And I can't see why they would fail in the way they have. I've added a commit to test out whether there is an issue with messages appearing on listeners from previous tests. This commit doesn't use the connection cache for tests. I don't intend for it to be merged - I'm just hoping to see if the tests return a different result. Could you run the github test actions on this PR again please |
I tried to run test on my mac and windows (without the last commit: But if it's related with cache. I think we need to separate the test to more small pieces. Like: one test case only to cover one test class/method, which makes it more maintainable. |
Wait... The GitHub Action runs on ubuntu, I thinking it's nothing todo with the Windows and SystemRoot stuff.
Is that because when locally run, the build server picks your gradle local installation(GRADLE_HOME env)? While I believe GitHub does not have that set. Is that because the gradle version used by |
No it should not related with the gradle version, because github pipeline only fails two cases. If it's due to the version, more cases should fail. But is that safe to call getTestDisaplayName()? Since it's a new API. |
I'm not sure. I don't know whether the we can use it because the tooling API is at 8.8 or whether Gradle also needs to be at 8.8. If I try different versions of Gradle locally I can't get |
It says:
See this commit: 4c01d1c, I forced the test project gradle version fix to 8.7 by adding the wrapper property file. Now more cases failed. Which highly like due to the API getTestDisplayName() I guess. |
I think maybe we need to:
|
I've wrapped Don't know how to disable tests purely on Github - is there an env var? |
Added an additional sourceset to test |
Here is says that "CI=true" will be set for github actions. I've disabled the 2 env var tests based on this. Also fixed formatting |
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.
LGTM
Thank you @Arthurm1 for your contribution. We can consider to support test debug delegation. Looks like BSP does not have that flag in request body. Maybe another extended point? |
@jdneo I think What's kind of interesting is that different clients might want different debug adapters....
I imagine the Java one is good enough for all but it would be handy to be able to specify the type in the BSP command and have the build server kick start the relevant one. Another thing to propose to add to BSP spec. |
Co-authored-by: Arthur McGibbon <[email protected]> Co-authored-by: Sheng Chen <[email protected]>
init named pipe test build Server named pipe connection update named pipe stream solution finish forward message finish merge and named pipe fix - Handle older versions of Gradle (microsoft#149) Co-authored-by: Arthur McGibbon <[email protected]> fix - Add JDK 22 compatibility support (microsoft#152) Signed-off-by: Sheng Chen <[email protected]> fix - Return the root cause of the message (microsoft#156) feat - Add support for running tests (microsoft#144) Co-authored-by: Arthur McGibbon <[email protected]> Co-authored-by: Sheng Chen <[email protected]> add Launcher by namedPipe with backwards compatible
Hi @Arthurm1, is the test feature working as expected on your Windows? I tested it today on Windows it's not working unless I remove |
@jdneo Yes it works for me running Are you on your |
Ah, I mean the code itself. Not test. I'm testing the integration with the gradle build server test support feature. Everything works fine one Mac. But on my windows, I have to comment Not sure if it's related with issue: gradle/gradle#29785. Or it's caused by my own environment. |
Ah - it works if I add
I should probably add this to |
Allows running of tests using
buildTarget/test
BSP requestThere appears to be no JVM specific
TestParamsDataKind
defined in the BSP spec, only Scala based ones. These should be fine to use on any JVM language (Java, Scala, Kotlin etc.)The following
TestParamsDataKind
types are supported...scala-test
which allows the client to run a list of tests at the class level per build target.scala-test-suites-selection
which allows the client to run a list of tests at the method level in a list of classes but only for 1 build target.There are a number of caveats to testing...
TaskStartParams
andTaskFinishParams
do not contain aLocation
as Gradle only supplies thesuite
,class
,method
for each test, not the file uri. That information is then lost as BSP has no way of reporting location except by file uri.TestFinish#message