Skip to content

Conversation

@Leibnizhu
Copy link
Contributor

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng
Copy link
Member

Does this PR continue from #169? I can see code names are very similar.

@wu-sheng
Copy link
Member

Does this PR continue from #169? I can see code names are very similar.

@Leibnizhu I need a confirmation about this.

@Leibnizhu
Copy link
Contributor Author

Does this PR continue from #169? I can see code names are very similar.

codes are copy from resteasy 3.x, and scenario test cases are from #169.

what i have done:

  1. correct the operation name made in SynchronousDispatcherInterceptor
  2. fixed some error assertion in expectedData.yaml
  3. the tests are passed in macos.

@wu-sheng
Copy link
Member

@carlfranz If you have time, I hope you could confirm this copy/paste doesn't concern you.

@Leibnizhu Thanks for the update. Let's focus on the codes and tests, once it is passed, and Carlo doesn't object, we should be good.

@wu-sheng
Copy link
Member

It seems UTs fail, could you recheck? Logs are here, https://github.com/apache/skywalking-java/runs/7107222730?check_suite_focus=true

@Leibnizhu
Copy link
Contributor Author

It seems UTs fail, could you recheck? Logs are here, https://github.com/apache/skywalking-java/runs/7107222730?check_suite_focus=true

i have fixed it , but ci action seems to be stucked……

@wu-sheng
Copy link
Member

It just waits for my manual approval, as this is your first pull request to this repo.

@wu-sheng
Copy link
Member

Your UTs are still failing, please make sure you have checked all cases locally.

@carlfranz
Copy link

If you have time, I hope you could confirm this copy/paste doesn't concern you.

Hi @wu-sheng, codes is pretty the same, as we saw before the plugin for resteasy4 differs only by the dependency artifactId and groupId.

@wu-sheng
Copy link
Member

If you have time, I hope you could confirm this copy/paste doesn't concern you.

Hi @wu-sheng, codes is pretty the same, as we saw before the plugin for resteasy4 differs only by the dependency artifactId and groupId.

Yes, I knew, as you mentioned in #169 (comment). This is the key point.
I just want to recheck with you, if this PR gets the tests passed, which were failed in the previous PR. Would you be OK if we get this merged? I don't want you to feel that your original codes(even they are test codes) are committed by another contributor. I want to be fair as much as possible.

1. Add defensive code SynchronousDispatcherInterceptor to avoid NPE
2. Add mocking for HttpRequest.getHttpMethod()
@Leibnizhu
Copy link
Contributor Author

Your UTs are still failing, please make sure you have checked all cases locally.

locally test pass:

  1. ./mvnw --batch-mode clean verify install for failed CI test Java 11 / ubuntu
  2. bash ./test/plugin/run.sh -f resteasy-4.x-scenario for resteasy-4.x plugin. tests for other plugins has passed in previous CI tests.

@carlfranz
Copy link

Would you be OK if we get this merged?

yes, there is no problem 😎

@wu-sheng wu-sheng added this to the 8.12.0 milestone Jun 29, 2022
@wu-sheng wu-sheng merged commit a60a61b into apache:main Jun 29, 2022
@Leibnizhu Leibnizhu mentioned this pull request Jul 27, 2023
5 tasks
@Leibnizhu Leibnizhu deleted the feat/resteasy4.x branch July 27, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants