| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
I think this is the more important point. I think this is a better approach: Fishrock123@34daef2 (Writable should allow things to overwrite it as if it were a regular value, I think?) |
Sorry, something went wrong.
|
Basically, minimizing the institutional knowledge, whether linted or not, is more important in the long run. |
Sorry, something went wrong.
|
(You'd still need these patches though I think.) Also this still only works if you refresh the tmpdir first, for I think obvious reasons. I'm not sure if that is practical to track. |
Sorry, something went wrong.
common.PIPE resides in the temp directory (except on Windows). Insure that the temp directory is refreshed in tests that use common.PIPE. Fixes: nodejs#3227
|
@Fishrock123 I'm trying to think of ways to decouple the use of common.PIPE from a need to refresh the temp directory. We could:
|
Sorry, something went wrong.
|
@jbergstroem has been talking about turning PIPE into a directory so we can parallelise more tests, there's likely some overlap with that discussion and this PR |
Sorry, something went wrong.
|
Re above: @Trott On top of your stuff, I'd like to expose an option to pass a directory in where the temporary directories/files are stored and create/clean that when starting the test runner. Once this is made we should just remove all logic related to NODE_COMMON_PIPE since that breaks parallel runs. We lean on this option in CI. |
Sorry, something went wrong.
|
Just for background on NODE_COMMON_PIPE for those editing it—it's there because the full path name can be too long for creating unix pipes which have a character limit, this is acute when run in Jenkins because the directories get deep and it's compounded by verbose naming of jobs and slaves. So on Jenkins we set it to ~/test.pipe (not actually ~) cause we know that's short enough. Not that I see any of this impacting on these changes, just wanted to make sure y'all have that in mind when messing around there. |
Sorry, something went wrong.
|
All of that sounds great to me. None of the above alters the need for changes in this PR, does it? |
Sorry, something went wrong.
|
@Trott Shouldn't. If so, I'll just modify post landing this. LGTM btw. |
Sorry, something went wrong.
common.PIPE resides in the temp directory (except on Windows). Insure that the temp directory is refreshed in tests that use common.PIPE. PR-URL: nodejs#3231 Fixes: nodejs#3227 Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
|
@Trott @jbergstroem @rvagg ... should this land in v4.x also? |
Sorry, something went wrong.
|
@jasnell It's a bugfix for tests only so... ¯_(ツ)_/¯ Probably fine either way. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes #3227
Except on Windows, common.PIPE is in the temp directory. So tests that use common.PIPE need to call common.refreshTmpDir(). I dislike the non-obvious coupling of those two things, and maybe there's a better solution, but for the moment at least, this fixes it.