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

Adjust integrationtests after #127 #376

Merged
merged 6 commits into from
Oct 16, 2024
Merged

Conversation

TobiasNx
Copy link
Collaborator

@TobiasNx TobiasNx commented Oct 14, 2024

Since Fix can create array with $append since #127. I thought we could simplify our integrationtests and delete all set_arrays that are only used to create empty arrays.

As result two integration tests seem to fail now.

  1. This seems to be an undetected bug, replace_all now copies an element it should not:

https://github.com/metafacture/metafacture-fix/blob/d18a5035d2b74508d685ec863c91e4382aff2c26/metafix/src/test/resources/org/metafacture/metafix/integration/method/fromXml/toJson/replace_allInOptionalSubfieldOfRepeatedObjectsWithAsterisk/test.fix

I opened a new issue for this: #377

  1. https://github.com/metafacture/metafacture-fix/blob/d18a5035d2b74508d685ec863c91e4382aff2c26/metafix/src/test/resources/org/metafacture/metafix/integration/record/fromJson/toJson/copy_fieldToArrayOfStringsWithIndex/test.fix

Catmandu creates new arrays with index numbers.

$ echo '{}' | catmandu convert JSON to YAML --fix 'add_field(list.0.b,test)'
---
list:
- b: test
...

$ echo '{}' | catmandu convert JSON to YAML --fix 'add_field(list.3.b,test)'
---
list:
- ~
- ~
- ~
- b: test
...

I am not sure if I am in favour of this behaviour since this blocks the possibilty of objects with numeric names.

If we want to support the behaviour for creating an array with index number we should revisit this test and change it.
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

I would prefer to leave the old integration test for method/fromXml/toJson/replace_allInOptionalSubfieldOfRepeatedObjectsWithAsterisk untouched and instead add a new one for #377.

Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

Agree about adding a new test, otherwise +1

@TobiasNx
Copy link
Collaborator Author

Done

Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

There's a typo in the path to the new test (Asteris -> Asterisk).

@TobiasNx
Copy link
Collaborator Author

Typo fixed

@TobiasNx TobiasNx requested a review from fsteeg October 16, 2024 08:52
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

The new test name could reuse the convention from the unit tests (ImplicitArray instead of WithoutCreatingArrayWithSetArray).

@blackwinter blackwinter removed their assignment Oct 16, 2024
@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Oct 16, 2024
@TobiasNx TobiasNx merged commit 9b4a6c1 into master Oct 16, 2024
1 check passed
@TobiasNx TobiasNx deleted the 127-adjustIntegrationTests branch October 16, 2024 11:53
@TobiasNx
Copy link
Collaborator Author

I changed a set_array specific integration test, I need to change it back: metafix/src/test/resources/org/metafacture/metafix/integration/record/fromJson/toJson/set_arrayWithoutValuesAndMove_fieldNewArrayOfStringsIntoArrayOfObjects/test.fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants