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

[tests] Rewrite all tests to do two passes and always write file results to disk before assertion checks - fixes #523 #528

Merged
merged 6 commits into from
Oct 31, 2021

Conversation

hazendaz
Copy link
Member

Fixes #523

Currently we do a single pass of formatting. This itself does not prove our formatting is actually valid. We need a second pass that processes the files formatted in the first pass to ensure they result in same output. This was needed to further track down issue with html processing and also flushed out issue in css processing.

@hazendaz hazendaz self-assigned this Oct 31, 2021
… per machine if not asked

Previously it was hard-coded to windows like which is invalid on linux/mac and only found when submitting more than one build for same.  Users affected would be users that are not setting defaults nor confirming what it is doing.
@hazendaz
Copy link
Member Author

@ctubbsii This one is a bit complicated and need second set of eyes here. Basically we only formatted one time per run. Unfortunately that does not prove the formatting is valid or will format same on secondary passes. Thus I added logic to run 2 passes for each type. For jsoup (html), there are too many issues so I disabled the second test run for that one on purpose. For the others, it became evident that were where more bugs in the tests on handling of mac/linux. This has been corrected there but wanted to have you review to see how this all looks.

After this, I think this provides proof our formatting is working as expected. The only open issue is jsoup. One issue there is when they will release patch that has fixes I submited and they merged to correct pretty printing we use on this. The next issue with jsoup is that it does not always remove trailing spaces. I added logic for that but secondary runs seem to add one extra whitespace character in any

block. So I'm still looking at that but think its an upstream issue given they failed to pretty print properly to start with and using that new feature to zap trailing spaces helps fix first pass but not second.

@hazendaz
Copy link
Member Author

there is possibly a bug in the line ending code in source. I think that has been questioned on a few times. The secondary format here changes nothing, that causes it to drop int a fixLineEnding logic. Going in, I think the prior test said use CRLF but then traps into this and since config didn't say, it goes to line ending auto, probably doesn't match and then goes to replace values. And that last bit then clearly doesn't match. I'm still a bit lost there but having dual tests now shows that our tests were not that great to start with. So flushing out a CSS issue and test issues all on a hunt to figure out why jsoup fails to format properly has brought this to light. For now, this itself doesn't change any behaviour as it is all test code, it just ensures what is the current process remains properly. For jsoup issues, I plan to raise some requests to them as to why things don't quite work in their pretty print.

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

@hazendaz I like the overall idea of two-passes. I have a follow-on PR to clean up some of the implementation, though, to make it more maintainable. So, I'll go ahead and merge yours, then mine.

@ctubbsii ctubbsii merged commit 1efe4df into revelc:main Oct 31, 2021
ctubbsii added a commit that referenced this pull request Oct 31, 2021
Refactor to make the two-pass testing more maintainable and easier to follow
@ctubbsii
Copy link
Member

there is possibly a bug in the line ending code in source. I think that has been questioned on a few times. The secondary format here changes nothing, that causes it to drop int a fixLineEnding logic. Going in, I think the prior test said use CRLF but then traps into this and since config didn't say, it goes to line ending auto, probably doesn't match and then goes to replace values. And that last bit then clearly doesn't match. I'm still a bit lost there but having dual tests now shows that our tests were not that great to start with. So flushing out a CSS issue and test issues all on a hunt to figure out why jsoup fails to format properly has brought this to light. For now, this itself doesn't change any behaviour as it is all test code, it just ensures what is the current process remains properly. For jsoup issues, I plan to raise some requests to them as to why things don't quite work in their pretty print.

I merged this PR, because I think the idea is solid. I then added my commit, 143f47f , which does some minor refactoring to remove some redundancy, and make the two-pass implementation substantially more readable and therefore more maintainable. Hopefully, that cleanup will help when going back to address any CSS/jsoup issues, or any other quirkiness with regard to line endings.

@ctubbsii ctubbsii added this to the 2.17.0 milestone Oct 31, 2021
@hazendaz
Copy link
Member Author

@ctubbsii Thanks! I was planning to come back in today to do some refactoring, I like what you have done here, it was even more than I was thinking of doing.

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

Successfully merging this pull request may close these issues.

Add 2 pases for all tests to ensure what was formated once remains formatted the same
2 participants