Skip to content

Split rwc input files#18772

Merged
weswigham merged 6 commits into
microsoft:masterfrom
weswigham:split-rwc-input-files
Sep 26, 2017
Merged

Split rwc input files#18772
weswigham merged 6 commits into
microsoft:masterfrom
weswigham:split-rwc-input-files

Conversation

@weswigham
Copy link
Copy Markdown
Member

@weswigham weswigham commented Sep 26, 2017

The last stage of the internal RWC format revision (prior to any rewrites for public consumption), this splits our RWC test definitions into a tree of files and a test definition json file. There is a linked internal PR with updated test definitions.

For anyone not familiar with this effort: this change should both make it much easier for us to audit changes to RWC tests when we recapture them (as we can review individual units of code), and also allows us to inspect the project structure on disk with our usual tools without a separate extraction step (though some projects may require extra local configuration to be navigable if they, for example, read their test settings from a response file) when debugging issues found by RWC tests.

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good, although I want to wait on merging it until somebody has a chance to make sure it works on Linux.

Comment thread Gulpfile.ts
gulp.task("tsc-instrumented", "Builds an instrumented tsc.js", ["local", loggedIOJsPath, instrumenterJsPath, servicesFile], (done) => {
exec(host, [instrumenterJsPath, "record", "iocapture", builtLocalCompiler], done, done);
gulp.task("tsc-instrumented", "Builds an instrumented tsc.js - run with --test=[testname]", ["local", loggedIOJsPath, instrumenterJsPath, servicesFile], (done) => {
const test = cmdLineOptions["tests"] || "iocapture";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be "test" not "tests"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or ... maybe gulp allows all prefixes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In gulp, "tests", "test", and "t" are all aliased to "tests" using the opt parsing library we use.

Comment thread src/harness/harness.ts
for (let {done, value} = gen.next(); !done; { done, value } = gen.next()) {
const [name, content, count] = value as [string, string, number | undefined];
if (count === 0) continue; // Allow error reporter to skip writing files without errors
const relativeFileName = relativeFileBase + (ts.startsWith(name, "/") ? "" : "/") + name + extension;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we now call sanitizeTestFilePath on relativeFileName somewhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We did before - the paths given to this function are always sanitized. What was changed was stripping the leading slash during sanitization, rather than here.

Comment thread src/harness/loggedIO.ts Outdated
wrapper.startReplayFromData(JSON.parse(logString));
};
wrapper.startReplayFromData = log => {
wrapper.startReplayFromData = (log) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why add parentheses here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can remove them, I experimented with some other modifications which added a parameter, but neglected to remove the parenthesis when I undid it.

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Sounds like you got a successful Linux run!

@weswigham weswigham merged commit 6ffee10 into microsoft:master Sep 26, 2017
@weswigham weswigham deleted the split-rwc-input-files branch September 26, 2017 22:56
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants