Skip to content
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

[ISSUE #4138] Add Unit Test for UrlMappingPattern #4139

Merged
merged 5 commits into from Jun 29, 2023

Conversation

Pil0tXia
Copy link
Member

Fixes #4138 .

Motivation

The TO-DO mark said that "It is better using Mockito not PowerMockito", so I used Mockito. Although there are private method/field to test in the compile() method of UrlMappingPattern.

Modifications

Use reflections to access private method and field, then write regular expressions.

The Test case passed.

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@Pil0tXia Pil0tXia requested a review from pandaapo June 21, 2023 04:24
Copy link
Contributor

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK for me after you remove the old comment // Verify that the compiledUrlMappingPattern field is updated.

@Pil0tXia
Copy link
Member Author

It's OK for me after you remove the old comment // Verify that the compiledUrlMappingPattern field is updated.

Of course, I have committed the changes.

I have a slight confusion in that although the assertNotNull() method does not verify that the compiledUrlMappingPattern matches the mockedPattern as the assertEquals() method did before, it still ensures that the compiledUrlMappingPattern has been updated by not being null.

Could you kindly enlighten me on the reasons behind your thoughts, if you don't mind?

@Pil0tXia Pil0tXia requested a review from pandaapo June 21, 2023 17:33
Copy link
Contributor

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, TestUrlMappingPattern. compiledUrlMappingPatternField is like a basic or goal, without the need for additional explanation. Additionally, I was also influenced by your previous code (which did update the compiledUrlMappingPatternField in this method).

But your reply reminded me that we can also consider TestUrlMappingPattern. compiledUrlMappingPatternField as part of the testing logic, rather than just a goal. Under this, that comment was actually OK.

@Pil0tXia
Copy link
Member Author

Thank you for your guidance! I agree with your viewpoint. I have reinstated this line of comment, and I would appreciate it if you could kindly approve it when you have a moment.

@Pil0tXia Pil0tXia requested a review from pandaapo June 22, 2023 09:38
Copy link
Contributor

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pil0tXia CI not pass, pls check it

@Pil0tXia
Copy link
Member Author

Pil0tXia commented Jun 26, 2023

@mxsm Hello! I have pulled the latest code from master to fix compile error and made modifications accordingly. 😉

@Pil0tXia Pil0tXia requested a review from mxsm June 26, 2023 07:18
Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Pil0tXia
Copy link
Member Author

PTAL😊 @xwm1992

@Pil0tXia
Copy link
Member Author

@lrhkobe PTAL😉

@xwm1992 xwm1992 merged commit 4e5428b into apache:master Jun 29, 2023
7 checks passed
@Pil0tXia Pil0tXia deleted the pil0txia_test_4138 branch January 4, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Unit Test] Add Unit Test for UrlMappingPattern
6 participants