Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the test by Madjosz · Pull Request #778 · stleary/JSON-java · GitHub
Skip to content

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

Merged
stleary merged 2 commits intostleary:masterfrom
Madjosz:fix_xml_test
Oct 8, 2023
Merged

Fix XMLTest.testIndentComplicatedJsonObjectWithArrayAndWithConfig() for Windows - in the test#778
stleary merged 2 commits intostleary:masterfrom
Madjosz:fix_xml_test

Conversation

@Madjosz
Copy link
Copy Markdown
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
Copy Markdown
Owner

stleary commented Oct 4, 2023

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Madjosz commented Oct 7, 2023

I rebased both branches ontop of the master for you.

@stleary
Copy link
Copy Markdown
Owner

stleary commented Oct 7, 2023

@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