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

Remove BOM in imports. #2391

Merged
merged 3 commits into from
Jan 15, 2015
Merged

Remove BOM in imports. #2391

merged 3 commits into from
Jan 15, 2015

Conversation

DotNetSparky
Copy link
Contributor

BOM bytes weren't being removed when @import'ing files, which can lead to syntax errors, etc.

As a bonus, this modifies the test functions to perform a second set of tests--a copy of the "test/less" folder is made as "test/less-bom", BOMs are inserted into each CSS and LESS file during the copy, and each test is ran for both folders. The copy is performed anytime the tests are ran if the less-bom folder doesn't exist or is empty.

Modified tests to run against a copy of the test files with BOMs inserted
into each .less and .css file (the copies are generated automatically if
the /test/less-bom folder doesn't exist or is empty).
@lukeapage
Copy link
Member

Nice. I thought we had bom removal - but maybe it was accidentally removed in v2.
thanks. Just one failing test - https://travis-ci.org/less/less.js/jobs/46945339

Some operations modify the options object, which could influence the next
test set.
Modified the test labels in the output to include the
folder (to distinguish between the normal and BOM tests).
Modified warning message from data-uri file-not-found to include the
filename.
@DotNetSparky
Copy link
Contributor Author

@lukeapage, yeah I just recently started having problems (I use less via WebEssentials for Visual Studio, and had a problem suddenly with autoprefixer crashing. I fixed the tests... turns out reusing the same options object between calls to less.render() is not safe... filemanager is storing paths in there (which was causing the failed test), and also explains a test failure I couldn't figure out (and so I had disabled the bom-version of the sourcemap test... I thought it was just my misunderstanding of how the test code works). it now makes a separate copies of the options object for the normal vs bom testsets, and everything is passing (including the bom-version of the sourcemap test).

I'm only making a copy of the options for each call to runTestSet(), I wonder if a copy should actually be made for each individual test (i.e. make the copy in the toCSS method before calling less.render). I didn't go ahead and do this, though, since I'm not sure if any of the testsets depend on that behavior...

@lukeapage
Copy link
Member

Thanks

@DotNetSparky
Copy link
Contributor Author

@lukeapage, one question for you... I made it so it only makes a copy of the test folder (adding the BOMs) if the folder doesn't already exist or is empty... but that means once it's made a copy, it won't auto-include any changes made to the tests if the folder isn't emptied. I'll take that out, so it always makes a copy, but I wasn't sure how the tests run automated (e.g. is it possible for multiple instances of "grunt test" being ran at the same time in the same folder)... ?

@lukeapage
Copy link
Member

but I wasn't sure how the tests run automated (e.g. is it possible for multiple instances of "grunt test" being ran at the same time in the same folder)... ?

No.. travis/ AppVeyor start from scratch, get from git and npm install, then test and clean up.

Useful if editing tests, so you don't have to keep deleting the
test/less-bom folder between each test.
@DotNetSparky
Copy link
Contributor Author

OK then, now it always makes the test copies (will make it easier when writing/editing tests). I think I'm done messing with the test scripts :)

lukeapage added a commit that referenced this pull request Jan 15, 2015
@lukeapage lukeapage merged commit ed1abbb into less:master Jan 15, 2015
@DotNetSparky DotNetSparky deleted the fix-bom-imports branch January 15, 2015 15:11
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.

2 participants