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

Test all input-files with short line-breaks #338

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

AndreasHoffmann2
Copy link

While using this library, we got some issues with line-breaks that are just waiting to occur in the real world.
After the second issue, we wrote a generic unit test.
This test takes all input files, minifies them with "-line:1" and tries to execute the minified code using the Microsoft.ClearScript-Engine. We found a total of four distinct issues when it comes to line-breaks:

  • OptionalChaining ('?.'): Line-break between '?' and '.' is not allowed. Already fixed in this PR.
  • ArrowFunctions ('=>'): Line-break between '()' and '=>' is not allowed. Already fixed in this PR.
  • yield-statement: if a line-break is added directly after the yield, JS returns undefined instead of the value. We know too little about NUglify to fix this.
  • async: It seems like there must not be a line-break after the keyword. Not yet fixed. We know too little about NUglify to fix this.

Additionally, the expected output for "ES6.js" simply does not compile and should be fixed.

Of course, the test ignores all js-files that are invalid from the start (e.g. because of placeholders).

How should we proceed with this?

Please find the current output of the test attached.

Test Output for SyntaxTestForAllFilesLineBreaks.Hipigog.txt

@trullock
Copy link
Owner

Thanks for these
Make a separate issue for the unfixed bits so we can address separately then I can merge the rest

@AndreasHoffmann2
Copy link
Author

I can create issues for the unfixed issues of course. But the unit-test will still fail, since it is a generic one that detects all five issues.
The code in this PR would remain unchanged then.
Is this ok?

@AndreasHoffmann2
Copy link
Author

About the failing check:
Restoring the packages works perfectly fine locally. I don't know how to fix it in AppVeyor.

@AndreasHoffmann2
Copy link
Author

Thank you Max for fixing the build. Now it's only the failing unit-test that makes the checks fail.
This is due to the open issues mentioned above (yield, async and ES6.js). These were in the project before this PR and are covered with a unit-test now.

@trullock
Copy link
Owner

I can probably fix the async thing later, I'll add it to this pr

@MaximKrasmik
Copy link

Is something missing in our request? Do you have a timeframe for the next release?

@trullock
Copy link
Owner

I'll take a look later, apologies

@trullock
Copy link
Owner

trullock commented Nov 8, 2022

When I come to run the tests for this I get:

System.TypeLoadException : Cannot load ClearScript V8 library. Load failure information for ClearScriptV8.win-x64.dll:
D:\git\NUglify\src\NUglify.Tests\bin\Debug\runtimes\win-x64\native\ClearScriptV8.win-x64.dll: The specified module could not be found
D:\git\NUglify\src\NUglify.Tests\bin\Debug\ClearScriptV8.win-x64.dll: The specified module could not be found

Nuget Restore doesn't fix this, is this right? Am I missing something?

@MaximKrasmik
Copy link

When I come to run the tests for this I get:

System.TypeLoadException : Cannot load ClearScript V8 library. Load failure information for ClearScriptV8.win-x64.dll:
D:\git\NUglify\src\NUglify.Tests\bin\Debug\runtimes\win-x64\native\ClearScriptV8.win-x64.dll: The specified module could not be found
D:\git\NUglify\src\NUglify.Tests\bin\Debug\ClearScriptV8.win-x64.dll: The specified module could not be found


Nuget Restore doesn't fix this, is this right? Am I missing something?

Some references in Nuglify.Tests.nuget.props depends from windows user profile and nugget version. This changes are not pushed.
Can u try to install missing packages manually? At least the paths stay the same.

  • Microsoft.ClearScript
  • Microsoft.ClearScript.V8

@AndreasHoffmann2
Copy link
Author

Please try it again with the new commit. It contains a new file NUglify.Tests.nuget.props.

@trullock
Copy link
Owner

trullock commented Dec 5, 2022

So, the tests fail, as per AppVeyor

@MaximKrasmik
Copy link

Which tests fail? We only fixed 2 of 4 problems for linebreaks, now the 2 others run into an error. The other failing tests we dont touched.

@AndreasHoffmann2
Copy link
Author

Just to avoid misunderstandings:
We added a unit-test that tests the js-minification with the most line-breaks possible. It does use all existing test-files for that.
Doing this, we found 4 already existing issues concerning the line-breaks.
Two of them were bothering us. And we got lucky: We were able to fix both of them. For the other 2, we lack the necessary knowledge. Thus these remain unfixed. But we left the unit-test failing to show the remaining issues.

TLDR:
We fixed two issues in NUglify and added a unit test that detects two more. We do not have the knowledge necessary to fix the other two issues. Thus we still improved the project a tiny bit without making anything else worse.

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.

3 participants