-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add a method to enable REPL-like debugging on e2e tests #21294
Conversation
@@ -112,6 +114,9 @@ function describeEnv(factory) { | |||
} | |||
} | |||
}); | |||
totalPromise = totalPromise.then(() => { |
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.
Could just return totalPromise.then
. Can you just turn this whole function into async/await instead?
for (const fixture of fixtures) {
await fixture.setup(env);
}
await installRepl(global, env);
If not, would consider:
let totalPromise = Promise.resolve();
fixtures.forEach((fixture) => {
totalPromise = totalPromise.then(() => fixture.setup(env));
});
return totalPromise.then(() => {
installRepl(global, env);
});
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.
That's a huge improvement. I was originally thinking about maintaining parity with the original describes.js
file, but I think the win here is worth it.
* Usage: in a test, await repl(); | ||
* @param {*} mochaThis | ||
*/ | ||
global.repl = function(mochaThis) { |
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.
Seems like you could have the describes-e2e code keep track of mochaThis
(I think inside of describe(SUB, function() {...}
, and then have this code just grab it. Then developers wouldn't need to change their arrow functions.
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.
Unfortunately I tried a few ways of using various mocha this
values, but the one from the currently running it
is the only this
that actually changes the timeout. Using cached this
values will cause the test to exit at 20000
ms, which is the default this
for the e2e test, which I assume is because the current it
's this
takes precedence?
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 suppose they have different this
values. Maybe there is a way to wrap it
, but I suppose we could always come back to it if we have time.
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.
Sounds good, it would be great to simplify this but I agree that iterating on it later would be best
build-system/tasks/e2e/repl.js
Outdated
}; | ||
|
||
function replContinue() { | ||
if (replResolve) { |
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.
consider changing to an early return
PTAL |
…1294) * Add a method to enable REPL-like debugging on e2e tests * Review feedback
…1294) * Add a method to enable REPL-like debugging on e2e tests * Review feedback
I got sick of debugging and having the test timeout and having to put a debugger statement and then not being able to call methods on the controller.
These calls are for debugging only and should not be checked in to the repo.
Demo (internal only): https://drive.google.com/file/d/1KtJjcy8o8oSdax1MChvCWGEpgYJFoco-/view?usp=sharing