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.

Sign up for GitHub

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: Fixes test-tick-processor to not hang on Windows #7628

Closed
wants to merge 5 commits into from
Closed

test: Fixes test-tick-processor to not hang on Windows #7628

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c35a880
test: fixes hang on Windows when running test-tick-processor in stres…
JoeDoyle23 Jul 9, 2016
f09ec99
Added missing newline
JoeDoyle23 Jul 9, 2016
f0829be
Adjusted tested function name and added comments about this test
JoeDoyle23 Jul 9, 2016
cc00183
Removed code file deletion and uncommented log deletion
JoeDoyle23 Jul 9, 2016
466876e
Fixed linting issues
JoeDoyle23 Jul 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions test/parallel/test-tick-processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
var fs = require('fs');
var path = require('path');
var assert = require('assert');
var cp = require('child_process');
var common = require('../common');
Expand All @@ -18,7 +19,7 @@ common.refreshTmpDir();
process.chdir(common.tmpDir);
// Unknown checked for to prevent flakiness, if pattern is not found,
// then a large number of unknown ticks should be present
runTest(/LazyCompile.*\[eval\]:1|.*% UNKNOWN/,
runTest(/LazyCompile.*test.js:1|.*% UNKNOWN/,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is designed to test the names of profiled functions appear in profiler output (as an approximation to profiler correctness). eval was used before since eval should appear on the stack every time it is observed by the profiler making it most likely to appear in the output. If the script below is now longer eval'd, we'll have to look for another symbol. f is too generic so maybe the function below could be renamed to something unique that the output could be checked for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh, so maybe the reason this makes it reliable is because the name of the file is always going to be there. I guess we'll see when the change is made.

Regardless, might be a good idea to preserve all this information-sharing in a comment or seven in the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In testing this, the file name appears in the same place that [eval] did in the original test's output, which I why I replaced it that way. It's not a big deal to make the function name more meaningful and then to adjust the regex if we think that's more declarative.

`function f() {
for (var i = 0; i < 1000000; i++) {
i++;
Expand All @@ -44,7 +45,9 @@ runTest(/RunInDebugContext/,
f();`);

function runTest(pattern, code) {
cp.execFileSync(process.execPath, ['-prof', '-pe', code]);
var testCodePath = path.join(common.tmpDir, "test.js");
fs.writeFileSync(testCodePath, code);
cp.execSync(process.execPath + ' -prof ' + testCodePath);
var matches = fs.readdirSync(common.tmpDir).filter(function(file) {
return /^isolate-/.test(file);
});
Expand All @@ -57,4 +60,5 @@ function runTest(pattern, code) {
{encoding: 'utf8'});
assert(pattern.test(out));
fs.unlinkSync(log);
fs.unlinkSync(testCodePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to remove the file, although I guess there's no harm either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have mis-commented before but this is backwards. The log file needs to be removed. The file in the tmp dir (testCodePath) does not. (The tmp directory is cleared by each test that needs it before it is used.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misread it. Fixed.

}