-
Notifications
You must be signed in to change notification settings - Fork 71
DAFFODIL-2539 Deduplicate TDML tests while preserving coverage for paths with spaces #1605
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
base: main
Are you sure you want to change the base?
Conversation
rename the TDML file for clarity, and retain coverage for schemas and TDML files located in paths with spaces. Signed-off-by: Tejas Tiwari <tt160705@outlook.com>
|
@mbeckerle here's the pull request for the issue DAFFODIL-2539 |
stevedlawrence
left a comment
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.
+1 with some minor updates.
Though we do need to fix support for spaces in file names, I think that might be currently broken in some cases. That can either be done as part of this PR if you want, or we can create a new ticket and fix that separately.
| correctly, and that duplicate global element names across schemas | ||
| with no namespaces result in a Schema Definition Error. |
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'm not sure this tests really wants to care about global element names across schemas. This only cares about spaces. That part of the description can probably be removed.
| --> | ||
|
|
||
| <tdml:parserTestCase | ||
| name="no_namespace_colliding_element_names" |
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 would suggest just calling this name="spaces_in_names_01" or something similar, to indicate this is just about verifing spaces in file names works. The fact that there are or aren't namespaces isn't really that important, namespaces just make for a more interesting test.
Make sure to update the test name in TestGeneral.scala to match.
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| --> | ||
|
|
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.
Since this changes the name of a .tdml file, it means we must also update the test that references the tdml file to use the new name. In this case that is in TestGeneral.scala:
So we should also update this test to use spacesInNames.tdml instead of namespaces.tdml and also update that test to reference the name of the parser test case.
However, that test in TestGeneral.scala expects it to not be able to find namespaces.tdml. Which it couldn't do before this change. That also probably means it won't be able to find spacesInNames.tdml when that is updated.
I think this means that the test is just broken--we should be able to support spaces in paths and we need tests that verify that works correctly but these tests do not work. I'm pretty sure spaces used to work at some point, so I'm not sure why this test expects failure now.
I think the issues is in our Misc class in the getResource* functions:
In both the functions we do replaceAll("""\s""", "%20") and then call Class.getResource on that path. But that is wrong because Class.getResource does not expect %20 and won't be able to find any resources that contain them. So our Misc.getResource* functions are broken with paths that contain spaces.
So these replaceAll's probably need to be removed or changed somehow. Note that there are probably other places that expect strings that are escaped URI's so we need to identify and fix those as well. I'm not sure exactly where those would be, but it would probably be revealed in testing.
Also note that there are characters other than spaces that need escaping in a URI. Most of those characters are uncommon in files or schema locations, but it does indicate that how we currently handle strings that are sometimes treated as URI's is probably not correct. We don't want to just add more replaceAlls, we probably need to think more carefully about how we handle strings and convert them to URI's. Maybe we need something like 'new URL(...).toURI` or something, since I believe URL's support spaces.
I think if you update the TestGeneral.scala file to replace namespaces.tdml with spaceInNames.tdml, that would be sufficient to remove the duplication as mentioned in the original ticket.
If you also want to attempt to fix the spaces issues, that could also be done as part of this ticket/PR or we can open another ticket to fix support for spaces.
Signed-off-by: Tejas Tiwari <tt160705@outlook.com>
|
Hey @stevedlawrence I did some of the fixes 1. Test intent clarification
2. Test reference alignment
3. Broken space-handling in resource lookup
4. Scope
With these changes, TDML duplication is removed, coverage for paths containing spaces is preserved, and the underlying resource resolution issue that caused the test confusion is fixed. |
|
I'll continue to fix the changes in this PR let me know if there are more originating issues with the recent changes @stevedlawrence |
|
Also @stevedlawrence, I would like to clarify that I ran the full sbt test suite, and the failures observed were not caused by the fix itself, but by tests that were asserting the previously broken behavior. Once the %20 encoding was removed from Misc.getResource*, classpath resource lookup for paths containing spaces began working correctly. This caused tests that were expecting “resource not found” errors to fail. Those tests were updated to validate successful resource resolution instead. |
|
It looks like there are two tests related to spaces in names that are failing. I don't think they are unrelated failures, but are due to additional changes needed to support spaces. One test is the new spaces_in_names_01 test, which fails with this stack trace: The "Caused by" part of the exception indicates we probably we have something like So it seems like we don't currently support spaces in the resolveSchemaLocation function. We may need further investigations to figure out how to correctly fix things and what other functions might need changes. I think the core issue is that some functions expect string paths to be valid URIs where spaces are escaped with %20 (e.g. resolveSchemaLocation, optRelativeFileURI, optRelativeResourceURI), but some expect them to be normal paths without escapes (e.g. getResourceOption, getRequiredResource) and some I'm not sure it's very well defined (e.g. getResourceAbsoluteOrRelativeOption). I'm sure there are other functions not listed that have some built-in assumptions. It feels like the right approach to convert our strings to Though, this could potentially have a pretty big cascading effect. Changing one variable to a Or alternatively, instead of changing things to URIs we just kindof have a convention that we expect these string paths to be valid URIs with escaped spaces (and sometimes we'll make escape things for the user), and the functions that don't want escapes can unescape the strings before use. So all this to say, I'm not really sure what the perfect approach is. It might require making some changes and see what gets affect, what tests start failing, etc. and use that to guide us to the best solution. You're welcome to dig into this and we're happy to help, but this might also be veering into an issue that isn't quite as beginner friendly since it might start touching a lot of the codebase and dealing with undocumented assumptions. |
This PR addresses DAFFODIL-2539 by refactoring TDML tests under
daffodil-test/src/test/resources/test space/test 1/.
The original namespaces.tdml duplicated coverage that already exists under
section06/namespaces, while also mixing multiple test concerns.
This change removes the duplicated TDML file and replaces it with a focused spaceInNames.tdml test that preserves coverage for schemas and TDML files located in directories with spaces, including schema resolution and no-namespace collision behavior.
All existing tests pass locally (sbt daffodil-test/test), and no functional coverage related to paths containing spaces is removed.