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

tools: remove unneeded temp directory handling #6094

Closed
wants to merge 3 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 7, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test tools

Description of change

The handling of temp directory creation happens in common.js now so
there is no need to test the creation of the temp directory in test.py.

@Trott Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 7, 2016
@Trott
Copy link
Member Author

Trott commented Apr 7, 2016

@joaocgreis
Copy link
Member

This was discussed in #3325 .

In common.js I found a mkdir for the test dir, but only the last one in the path (just the thread number if testing with -J). Is there one for the whole path?

@jbergstroem
Copy link
Member

@Trott: can you point me to where it's happening? Afaik its only the thread folders, not the actual dir the thread folders lives in.

@Trott
Copy link
Member Author

Trott commented Apr 8, 2016

Is there one for the whole path?

No, but I can add one as part of this PR, and then see how review of this PR goes. (I assume there's general support for reducing the amount of Python code we rely on in exchange for more JavaScript code, all other things being equal.)

Trott added 2 commits April 7, 2016 21:14
The handling of temp directory creation happens in common.js now so
there is no need to test the creation of the temp directory in test.py.
@Trott
Copy link
Member Author

Trott commented Apr 8, 2016

@joaocgreis OK, mkdirp implemented as part of common.js. Worked for me when I changed common.tmpDir to have a few extra 'tmp's in the path. In other words:

exports.tmpDir = path.join(testRoot, 'tmp', 'tmp', exports.tmpDirName);

PTAL

@Trott
Copy link
Member Author

Trott commented Apr 8, 2016

@jbergstroem If I'm understanding your question correctly, the mkdirp() implementation is now where that will happen.

@joaocgreis
Copy link
Member

I assume there's general support for reducing the amount of Python code we rely on in exchange for more JavaScript code

That is not my opinion. Even if our test runner is somewhat confusing (to be nice), I like having something very stable to run the tests, because the node executable we test can be somewhere between release quality and completely broken.

@jbergstroem
Copy link
Member

@Trott yes, but that also assumes that the parent directory exists. I actually used that same code path in a prior version of the patch but it ended up being much cleaner when implemented in test.py.

Edit: didn't see your patch. I'm kind of "if it works..." with this to be honest. Happy to replace test.py at some stage but I don't see a big win in moving it to common.js.

@Trott
Copy link
Member Author

Trott commented Apr 14, 2016

Sounds like there's support for simplifying/improving/replacing test.py but that this is not a win. Closing.

@Trott Trott closed this Apr 14, 2016
@Trott Trott deleted the rool branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants