Skip to content

Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the test #778

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.

Sign up for GitHub

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

Jump to bottom
Merged
stleary merged 2 commits into from
Oct 8, 2023
Merged

Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the test #778

stleary merged 2 commits into from
Oct 8, 2023

Conversation

@Madjosz
Copy link
Contributor

@Madjosz Madjosz commented Oct 4, 2023

#593 and 85495fa included some workaround for Windows machines regarding linefeeds. The test org.json.junit.XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() is currently failing on my Windows 10 machine and also I see no reason why this workaround was needed in the first place. The file Issue593.xml contains only \n line feeds and no conversion is done while reading this file, thus expected only contains \n. Conversely the tested method XML.toString() only appends hardcoded \n.

The only setting where the conversion is needed is when someone has their .gitattributes/Git settings or IDE configured to autoreplace linefeeds by the system default. In that case I would say it is actually correct that the test fails because it indicates a wrong setup.

@stleary
Copy link
Owner

stleary commented Oct 4, 2023

@Madjosz Thanks for catching this! I had gotten out of the habit of testing on Windows.

@stleary stleary changed the title fix failing test XML test on Windows machines Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the code Oct 6, 2023
@stleary stleary changed the title Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the code Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the test Oct 6, 2023
@stleary
Copy link
Owner

stleary commented Oct 6, 2023 •
edited
Loading

@Madjosz I prefer this version of the fix, but I cannot test it locally, probably because of recent commits. You will need to fix this if you want your PR to be considered.

git pull https://github.com/Madjosz/JSON-java.git fix_xml_test
From https://github.com/Madjosz/JSON-java
* branch fix_xml_test -> FETCH_HEAD
fatal: Not possible to fast-forward, aborting.

Never mind, it's working now, but I had to run: git config pull.rebase false

@johnjaylward
Copy link
Contributor

johnjaylward commented Oct 6, 2023

@stleary THE PR doesn't say there is a conflict. Are you sure you are on master when trying the pull?

stleary
stleary previously approved these changes Oct 7, 2023
View reviewed changes
@stleary
Copy link
Owner

stleary commented Oct 7, 2023

What problem does this code solve?
Fixes a unit test that somehow became broken in Windows

Risks
Low

Changes to the API?
N/A

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
N/A

Was any code refactored in this commit?
N/A

Review status
APPROVED

Because of Hacktoberfest, both #782 and #778 will be accepted and merged, in that order.

Starting 3-day comment window

@Madjosz
Copy link
Contributor Author

Madjosz commented Oct 7, 2023

I rebased both branches ontop of the master for you.

@stleary
Copy link
Owner

stleary commented Oct 7, 2023

@Madjosz Thanks, checkout and pull is working now.

@stleary stleary dismissed their stale review October 8, 2023 22:03

The merge-base changed after approval.

@stleary stleary merged commit 776b5cc into stleary:master Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stleary stleary stleary approved these changes

Assignees

No one assigned

Labels

hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Madjosz @stleary @johnjaylward