Skip to content

Conversation

@sonalgaud12
Copy link

@sonalgaud12 sonalgaud12 commented Mar 27, 2025

This PR addresses #523 which comes under utilities class

Added Test:

  • testNoHashOrParameters: Verifies file name extraction from a simple file path without parameters
  • testEmptyString: Checks handling of empty input string
  • testOnlyHash: Tests retrieving file name from a URL with a hash fragment
  • testOnlyQueryParams: Validates file name extraction from a URL with query parameters
  • testHashAndQueryParams: Ensures correct file name parsing from a URL with both hash and query parameters
  • testGetIndexOfParamsNoParams: Confirms parameter index detection for URL without parameters
  • testGetIndexOfParamsHash: Checks parameter index identification in a hash fragment
  • testGetIndexOfQueryHash: Validates parameter index detection in query parameters
  • testGetIndexOfBoth: Verifies parameter index extraction from a URL with hash and query parameters
  • testGetParamsNone: Ensures no parameter extraction from a simple URL
  • testGetParamsHash: Tests parameter extraction from a URL with a hash fragment
  • testGetParamsQuery: Validates parameter extraction from a URL with query parameters
  • testGetParamsBoth: Checks parameter extraction from a URL with both hash and query parameters

Result:

image

Copy link
Member

@wbrunette wbrunette left a comment

Choose a reason for hiding this comment

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

There appears to be a lot of constant strings. It is better to list them as constants at the top and then use them to separate logic from declaration of constants.

For example EMPTY_STRING = "";
URL_SEGMENT1 = "test/"

@sonalgaud12
Copy link
Author

Thank you for the feedback, I have made the changes.

@sonalgaud12 sonalgaud12 requested a review from wbrunette March 27, 2025 18:24
@sonalgaud12 sonalgaud12 requested a review from wbrunette April 8, 2025 11:50
* @param segment URL segment to test
* @param expected Expected index of parameters
*/
protected void assertGetIndexHelper(String segment, int expected) {
Copy link
Member

Choose a reason for hiding this comment

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

If you change the package to org.opendatakit.webkitserver.utilities you should not need to use reflection.

Copy link
Author

Choose a reason for hiding this comment

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

I tried this approach but encountered access errors. Even with the test in the same package name, I got the error: 'getIndexOfParameters(java.lang.String)' is not public in 'org.opendatakit.webkitserver.utilities.UrlUtils'. It cannot be accessed from outside the package.
I could add a public helper method in the UrlUtils class specifically for testing or use reflection. Please let me know if you'd like me to try a different approach for accessing the package-private method

Copy link
Member

Choose a reason for hiding this comment

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

This conversation should not be resolved because it was not resolved. Also, generally, the person doing the review resolves when it's complete.

The package you changed it to is NOT the same:
org.opendatakit.webkitserver.utilities; vs org.opendatakit.test.webkitserver.utilities;

It needs to be exactly the same: org.opendatakit.webkitserver.utilities;

As stated in the review.

Copy link
Author

Choose a reason for hiding this comment

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

Relocated UrlUtilsTest to match the package structure of the class under test. To allows access to package-private methods for more thorough testing.

@sonalgaud12 sonalgaud12 requested a review from wbrunette April 8, 2025 16:43
Copy link
Member

@wbrunette wbrunette left a comment

Choose a reason for hiding this comment

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

Suggestions form last code review not fixed.

A package-private function should be available to a class in the same package.

https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html

@sonalgaud12 sonalgaud12 requested a review from wbrunette April 16, 2025 06:44
Copy link
Member

@wbrunette wbrunette left a comment

Choose a reason for hiding this comment

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

You should not delete basictest

@sonalgaud12 sonalgaud12 requested a review from wbrunette May 24, 2025 08:55
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.

2 participants