You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "kezhenxu94 (via GitHub)" <gi...@apache.org> on 2023/02/12 13:36:40 UTC
[GitHub] [skywalking] kezhenxu94 opened a new pull request, #10382: Migrate tests from junit 4 to junit 5
kezhenxu94 opened a new pull request, #10382:
URL: https://github.com/apache/skywalking/pull/10382
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #[<issue number>](https://github.com/apache/skywalking/issues/8551).
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427375501
Great. Once all CI passed, I am good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427469502
Maven profile could be activated based on JDK version. Does it help your case to inject compiler parameters? Such as through a property value.
```
<profile>
<id>jdk-versions</id>
<activation>
<jdk>(1.4, 8]</jdk>
</activation>
------
</profile>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427588939
> > > I also remove the `backend`, `ui` profiles, we seldom rely on those two profiles, and if users want to only package backend or UI module, there are a bunch of maven-way to do that, such as `./mvnw -f oap-server/pom.xml package` or `./mvnw -pl :oap-server -am package`, I've also updated the related doc and Makefile.
> >
> >
> > I think there is an issue, because dist profile is missed.
>
> dist profile includes apm-dist module, and apm-dist module is added to the root `<modules>` already, so I remove `dist` profile, anything else?
The previous `-Pbackend,dist` means using the existing UI and new packaged backend to build dist. Right now, in `./mvnw -f oap-server/pom.xml package` case, how maven could know this? I feel UI would be recompiled again too, as it is on the module list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427391614
We can compile and test in Java 17, but currently some of the tests will fail in Java 17, as that requires some JVM arguments available only in Java version > 8, like `--add-opens`, this is hard to make it in both Java 8 and Java 17, I'll do in another PR to move the minimum requirement to Java 11 after discussed and voted in mailing list, for now this PR is good to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427532944
> I know that, but it is not worth to do this as we don't plan to support both java 8 and java 17, I mean we should have only one recommended development JDK version, right?
If it is not too complex, we could have JDK 8/11/17 all supported.
> Using the jdk profile, you have to explicitly specify the other profile such as -Pdist, when jdk8 profile is automatically activated, the other profiles are not activated even they have <activeByDefault>true</activeByDefault>.
This is a range for the JDK version, I think we only need one profile for JDK 8 and the other for JDK 9+.
The `<jdk>` is inside `<activation>`, isn't it automatically picked up in different JDK versions? I am guessing, but otherwise, what is the point of this configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427579697
> > I also remove the `backend`, `ui` profiles, we seldom rely on those two profiles, and if users want to only package backend or UI module, there are a bunch of maven-way to do that, such as `./mvnw -f oap-server/pom.xml package` or `./mvnw -pl :oap-server -am package`, I've also updated the related doc and Makefile.
>
> I think there is an issue, because dist profile is missed.
dist profile includes apm-dist module, and apm-dist module is added to the root `<modules>` already, anything else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427531861
OK I think I can remove some of the original profiles as they are always activated by default, now I can add JDK profile, let's see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#discussion_r1104154979
##########
pom.xml:
##########
@@ -33,6 +33,10 @@
<modules>
<module>apm-protocol</module>
+ <module>oap-server</module>
+ <module>oap-server-bom</module>
+ <module>apm-webapp</module>
+ <module>apm-dist</module>
Review Comment:
> This is the reason we could close `activeByDefault` for those modules, right?
Yes, and because these modules are added in the previous default profiles, so I think this is OK we just make it default here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427505616
> Maven profile could be activated based on JDK version. Does it help your case to inject compiler parameters? Such as through a property value.
>
> ```
> <profile>
> <id>jdk-versions</id>
> <activation>
> <jdk>(1.4, 8]</jdk>
> </activation>
> ------
> </profile>
> ```
I know that, but it is not worth to do this as we don't plan to support both java 8 and java 17.
Using the jdk profile, you have to explicitly specify the other profile such as `-Pdist`, when jdk8 profile is automatically activated, the other profiles are not activated even they have `<activeByDefault>true</activeByDefault>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427540078
OK, I see the point now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427691276
I've run out of ideas, just reverted back the changes, currently only Java 8 is supported out of the box.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427368617
> Is this SkyWalking being able to compile in JDK17?
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427563601
I also remove the `backend`, `ui` profiles, we seldom rely on those two profiles, and if users want to only package backend or UI module, there are a bunch of maven-way to do that, such as `./mvnw -f oap-server/pom.xml package` or `./mvnw -pl :oap-server -am package`, I've also updated the related doc and Makefile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427334587
Is this SkyWalking being able to compile in JDK17?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427635065
Well, things become complex now, there are profiles inside `apm-dist` module...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427539360
> isn't it automatically picked up in different JDK versions? I am guessing, but otherwise, what is the point of this configuration.
It's automatically picked up, but the other profiles are deactivated, even they have `<activeByDefault>true</activeByDefault>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#discussion_r1104151003
##########
pom.xml:
##########
@@ -33,6 +33,10 @@
<modules>
<module>apm-protocol</module>
+ <module>oap-server</module>
+ <module>oap-server-bom</module>
+ <module>apm-webapp</module>
+ <module>apm-dist</module>
Review Comment:
This is the reason we could close `activeByDefault` for those modules, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427570118
> I also remove the `backend`, `ui` profiles, we seldom rely on those two profiles, and if users want to only package backend or UI module, there are a bunch of maven-way to do that, such as `./mvnw -f oap-server/pom.xml package` or `./mvnw -pl :oap-server -am package`, I've also updated the related doc and Makefile.
I think there is an issue, because dist profile is missed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10382:
URL: https://github.com/apache/skywalking/pull/10382#issuecomment-1427633179
> Right now, in ./mvnw -f oap-server/pom.xml package case, how maven could know this? I feel UI would be recompiled again too, as it is on the module list.
Hi, you can translate the command `./mvnw -pl oap-server -pl apm-dist` as:
```xml
<modules>
<module>oap-server</module>
<module>apm-dist</module>
</modules>
```
UI won't be compiled again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #10382: Migrate tests from junit 4 to junit 5
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #10382:
URL: https://github.com/apache/skywalking/pull/10382
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org