-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
[test] refactor serialization suite #4777
[test] refactor serialization suite #4777
Conversation
If this PR gets merged first, then #4773 can be closed. |
c7aff62
to
1004a29
Compare
This PR was rebased on current develop HEAD and does not include #4773 anymore |
we have a problem with the docker images it seems (although this is completely unrelated to my changes):
@duncdrum do you have an idea what this is about or what might be the reason? Is this just a temporary hiccup and I should try again in a moment? |
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.
Please consider that the following change is not equivalent:
Before this PR we had:
map { xs:QName("exist:output-doctype"): fn:false() }
But now you have:
map { "exist:output-doctype": false() })
Whilst the end result may be the same, we added the functionality and designed the tests to specifically test for the QName, this should not be removed please.
@line-o no clue could be a hickup. I would recommend a local build of the image, but that only works on intel so … |
I see that not all such QName constructions were removed - e.g., https://github.com/eXist-db/exist/pull/4777/files#diff-41536ff061981a585744759c566780f801e29b78405dd79d158d466f3272905dR374. I agree that it would be best to restore the others to their original form with the xs:QName construction. Perhaps the versions without the xs:QName construction could be preserved too as variations to ensure that both forms continue to work (as they do today). |
I would be in favour of adding one test that uses constructed QNames to ensure this form will continue to work. |
Kudos, SonarCloud Quality Gate passed! |
Not for us. Please keep our QName tests, we worked hard to add this and ensure that it works for several groups of users. If you wish to add additional tests for strings as well, I have no objection, but please do not remove our tests! |
65ad971
to
b0ff9b3
Compare
@adamretter all exist-specific serialization option keys are now tested as |
@adamretter out of curiosity: is it possible to use QNames for the other keys in the map as well? |
@dizzzz could you remove the build cache for this PR so that we can re-run the tests? |
- combine item-separator with method tests into one `ser:item-separator-with-method` - simplify test cases that used eq in combination with assertXPath - make direct calls to function under test (serialize) in each test - extract fixtures to module variables - remove namespace from children of output:serialization-parameters - convert QNames keys to strings for exist-db specific options - convert string constructors to CDATA sections - make HTML5 test data real world examples - use unprefixed form of built-ins - fix formatting
eXist-db specific extensions to serialization options can also be set using the QName instead of plain strings. Add one test per option using the non-default value.
The value for each serialization option is now paremeterized.
b0ff9b3
to
6735560
Compare
Kudos, SonarCloud Quality Gate passed! |
@adamretter Your concerns were addressed 2 months ago, would you please re-review this PR |
I will bring this PR up on the community call today. |
Description:
Refactor serialization tests for maintainability and readability.
ser:item-separator-with-method
Reference:
has to wait for
Type of tests:
XQSuite