Deduplicate TDML tests while preserving coverage for paths with spaces #1605
Conversation
|
@mbeckerle here's the pull request for the issue DAFFODIL-2539 |
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+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.
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.
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.
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.
|
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. |
|
I agree @stevedlawrence this PR has surfaced a deeper inconsistency in how paths versus URIs are handled (notably around schemaLocation and related resolution logic). |
|
I would suggest reverting the changes in Misc.scala and change the test in TestGeneral.scala back so that expects a "not found" error (but expects the new spacesInNames.tdml file). This way this PR is limited in scope to only removing the duplicate namespaces.tdml file. Then we can open a new ticket about spaces in paths not working correctly and we can reference this PR and the test you've added in the ticket. |
|
@stevedlawrence With this, the scope of the PR is limited strictly to DAFFODIL-2539 (deduplicating TDML tests while preserving coverage), without attempting to fix the broader spaces-in-paths behavior. |
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1 👍 looks good with just a couple more minor updates.
Can you also squash your commits (including the suggested changes) into a single commit and update the commit message to follow the style mentioned in the code contributor workflow. Note that for the Daffodil project we don't usually put the jira issue in the title, but instead put it at the end of the commit message.
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Please undo this change, files should end with a new line. This might require a change in your IDE settings.
| @Test def invalidRestrictionPolicyValidate_01 = test | ||
| @Test def invalidRestrictionPolicyValidate_02 = test | ||
| } | ||
| } |
There was a problem hiding this comment.
Please also add a newline to this file.
| object TestSpace2 extends TdmlSuite { | ||
| val tdmlResource = "/test space/test 1/namespaces.tdml" | ||
| } | ||
|
|
||
| class TestSpace2 extends TdmlTests { | ||
| val tdmlSuite = TestSpace2 | ||
|
|
||
| @Test def no_namespace_02(): Unit = { | ||
| val e = intercept[Exception] { test } | ||
| val m = e.getMessage() | ||
| assertTrue(m.toLowerCase.contains("required resource")) | ||
| assertTrue(m.contains("/test%20space/test%201/namespaces.tdml")) | ||
| assertTrue(m.toLowerCase.contains("not found")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please change the tdmlResource to reference spacesInNames.tdml and in the assertTrue. When we fix the spaces issue it will make it clear where the tdml file is to be used for testing.
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> Fix classpath resource lookup for paths containing spaces Signed-off-by: Tejas Tiwari <tt160705@outlook.com> Revert resource lookup behavior to keep PR scoped to test deduplication. Signed-off-by: Tejas Tiwari <tt160705@outlook.com> Fix formatting and update TestGeneral references Signed-off-by: Tejas Tiwari <tt160705@outlook.com>
Ontiomacer
force-pushed
the
daffodil-2539-tdml-dup-cleanup
branch
from
772ff8c to
b2f4f28
Compare
Ontiomacer
changed the title
|
@stevedlawrence |
|
“Allow edits by maintainers” has been enabled to make it easier to apply |
2 participants
Deduplicate TDML tests under test space/test 1
Remove duplicated TDML coverage by deleting namespaces.tdml and
replacing it with a focused spaceInNames.tdml test. This preserves
coverage for TDML and schema resources located in directories
containing spaces without changing existing behavior.
DAFFODIL-2539