-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: adds utility launch for debugging a single test file #4432
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
Conversation
|
There are a couple of instances (e.g. debugging |
efad088 to
adf202e
Compare
|
NOTE: I have rebased from upstream and squashed all commits, which should address the travis commitlint and doc errors. |
adf202e to
9e4dbb2
Compare
9e4dbb2 to
45b825d
Compare
45b825d to
f7f7fb6
Compare
f7f7fb6 to
1f434f2
Compare
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The changes look reasonably, I have two small comments to address.
|
Sorry for the delay in getting back to this PR! I had a brief vacation. Will update and address things :) |
1f434f2 to
a03b815
Compare
|
With approvals from @raymondfeng and @bajtos I'd be happy to merge this in :) |
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome back, @mschnee. I am looking forward to see this improvement landed 🤸♂️
I am afraid I found few details that I am not very happy with, can you please fix them? See my comments below and also Raymond's comment about wrong copyright year(s) in the license header.
Thank you!
6df5740 to
aec9eeb
Compare
6b2b1e0 to
2d91f92
Compare
|
Addressing the commitlint changes :) |
2d91f92 to
68a5d53
Compare
derdeka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Thanks, works well for me with Windows 10.
feat(build): adds launch for debugging a single test file
nitpick: Please change the commit message, as it does not effekt the build package anymore.
|
@derdeka What's the correct scope? |
68a5d53 to
5e19d8c
Compare
In this case, I think it's best to leave the scope out. Also please note that we are using imperative form ("add something"), not present tense ("adds something"). See subject in our Commit Message Guidelines and also A Note About Git Commit Messages. I am proposing the following commit message: (Personally, I would choose |
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
Please update the commit message per my comment above before landing.
| const relative = path.relative(currentDir, absolutePath); | ||
| const resultPath = relative.replace(/^src/, 'dist').replace(/\.ts$/, '.js'); | ||
| return path.resolve(base, resultPath); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: the current code has a theoretical possibility of infinite loop as it does not test the root of a file system. A more robust implementation can be:
let pkgFile = path.resolve(filename, '../package.json');
// Find root directory of the file system
const root = path.parse(pkgFile).root;
while (!fs.existsSync(pkgFile)) {
if (path.dirname(pkgFile) === root) {
// We reach the root of file system
pkgFile = undefined;
break;
}
// Try the parent directory level package.json
pkgFile = path.resolve(pkgFile, '../../package.json');
}
if (!pkgFile) return filename; // Or throw an error
const projectRoot = path.dirname(pkgFile);
const relative = path.relative(projectRoot, path.resolve(filename));
const jsFilePath = relative.replace(/^src/, 'dist').replace(/\.ts$/, '.js');
return path.resolve(projectRoot, jsFilePath);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a small change to the current function, and tested it by invoking this cli against both a test file inside of the loopback-next workspace, and also a fake.test.js file that has no package.json file in it's filesystem heirarchy
5e19d8c to
9b10153
Compare
9b10153 to
e4acd65
Compare
Partially implements: #4300
See: #4406
This has been split from #4406 as a more manageable, smaller PR.
This update adds a developer convenience Launch task to allow debugging a single unit test file in Visual Studio Code.
Adds
bin/mocha-current-file.js- this is a utility script that attempts to determine the dist output of the given test file, and passes it off to mocha in-process.Modifies
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated