Add unit tests for the bundled compiler and compressor classes #531
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fdintino
force-pushed
the
pr/compiler-compressor-unit-tests
branch
2 times, most recently
from
41865f0 to
c8c4f3f
Compare
|
@jezdez Any feedback on this? |
|
I think @cyberdelia would be better reviewing this from a conceptual point of view. I could only from a Python perspective tbh. |
|
@cyberdelia could you give some guidance? |
| @@ -97,7 +98,7 @@ def __getitem__(self, key): | |||
| value = self.settings[key] | |||
| if key.endswith(("_BINARY", "_ARGUMENTS")): | |||
| if isinstance(value, string_types): | |||
| return tuple(shlex.split(value)) | |||
| return tuple(shlex.split(value, posix=(os.sep == '/'))) | |||
There was a problem hiding this comment.
Perhaps use (os.name == 'posix') instead?
There was a problem hiding this comment.
I had done this because I wasn't sure if any of the other possible values for os.name had posix-like paths. But it doesn't appear that they do, so that's functionally equivalent and definitely clearer.
fdintino
force-pushed
the
pr/compiler-compressor-unit-tests
branch
from
c8c4f3f to
d8af869
Compare
davidt
added a commit
to davidt/django-pipeline
that referenced
this pull request
Feb 22, 2016
Pull request 531, among many other things, includes a similar fix for the use of `stdout` before it's been assigned. That implementation is a nicer read, so I've switched over to it.
shlex.split(), without posix=False, strips "\" characters, which is obviously problematic on windows.
fdintino
force-pushed
the
pr/compiler-compressor-unit-tests
branch
2 times, most recently
from
b71dd5e to
f03a96a
Compare
| @@ -54,3 +62,12 @@ def relpath(path, start=posixpath.curdir): | |||
| if not rel_list: | |||
| return posixpath.curdir | |||
| return posixpath.join(*rel_list) | |||
|
|
|||
|
|
|||
| def set_std_streams_blocking(): | |||
There was a problem hiding this comment.
It would be nice if this had a docstring explaining what it does and why it's needed.
Some nodejs executables set stdout and/or stderr to non-blocking, which can trigger an IOError when the collectstatic command prints more than 1024 bytes at a time to stdout or stderr
If this environment variable is not set, multiprocessing throws a NotImplementedError
fdintino
force-pushed
the
pr/compiler-compressor-unit-tests
branch
from
f03a96a to
c17fd82
Compare
|
Looks good! |
davidt
added a commit
that referenced
this pull request
Feb 22, 2016
…tests Add unit tests for the bundled compiler and compressor classes
|
Thanks! I'll get to work adding unit tests and documentation to my source map pull request (#490) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to write unit tests for my sourcemap pull request (#490) I needed to have a way in the current testing system to run tests against the possible compilers and compressors available (e.g. scss, closure, uglifyjs). If node is available on the system tox installs npm packages into a local
node_modulesfolder, and the settings then use the binaries in that local folder. YUI and closure also check for a java executable in the PATH, and csstidy does not have an npm package so it is the only compiler/compressor that doesn't get bootstrapped in the tox tests.I noticed that you have made some effort to have the unit tests pass on windows, so I kept that in mind here. In the process of writing these tests I found some bugs, the fixes to which I've included in this pull request as separate commits.