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

feat: support checkpoints for one-step replay debugger #3410

Merged
merged 12 commits into from
Jul 28, 2021

Conversation

violetyao
Copy link
Contributor

What does this PR do?

As part of the one-step replay debugger command, this PR checks for the presence of checkpoints. If they are present, this PR uploads them before we execute the Apex Test(s). Checkpoints have a limit of 5 per org/user, if the limit is exceeded, the command execution would be canceled and a message indicating this limit violation would be shown to the user.

What issues does this PR fix or reference?

@W-7757402@

Functionality Before

The one-step replay debugger command did not support checkpoints.

Functionality After

The one-step replay debugger command uploads checkpoints if they are present.
Screen Shot 2021-07-12 at 11 12 08 AM
Screen Shot 2021-07-12 at 11 11 47 AM

If there are more than five checkpoints, the command executed is canceled and a message is reported to the user.
Screen Shot 2021-07-12 at 11 11 07 AM

@violetyao violetyao requested a review from a team as a code owner July 12, 2021 18:13
@violetyao violetyao requested a review from jag-j July 12, 2021 18:13
Copy link
Collaborator

@AnanyaJha AnanyaJha left a comment

Choose a reason for hiding this comment

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

Small comment, otherwise it looks good!

@AnanyaJha
Copy link
Collaborator

What's the behavior if you don't have any checkpoints? Do we still need to run this command? Is it possible to check whether checkpoints are present beforehand?

@violetyao
Copy link
Contributor Author

violetyao commented Jul 21, 2021

What's the behavior if you don't have any checkpoints? Do we still need to run this command? Is it possible to check whether checkpoints are present beforehand?

Thank you for reviewing, Ananya! We discussed in one of our daily syncs and decided to close @W-7748717@ because the debug & run command should actually not give warnings (should just run through the test) if there is no checkpoint/breakpoint.

We use SFDX: Update Checkpoints in Org to check if there is any checkpoint available and to upload checkpoints if they exist. If there is no checkpoint, the behavior of the debug & run button is the same with directly running tests using the green run button, except that those SFDX: Update Checkpoints in Org messages are printed. I've attached a screenshot below!

Screen Shot 2021-07-21 at 12 57 58 PM

@AnanyaJha
Copy link
Collaborator

What's the behavior if you don't have any checkpoints? Do we still need to run this command? Is it possible to check whether checkpoints are present beforehand?

Thank you for reviewing, Ananya! We discussed in one of our daily syncs and decided to close @W-7748717@ because the debug & run command should actually not give warnings (should just run through the test) if there is no checkpoint/breakpoint.

We use SFDX: Update Checkpoints in Org to check if there is any checkpoint available and to upload checkpoints if they exist. If there is no checkpoint, the behavior of the debug & run button is the same with directly running tests using the green run button, except that those SFDX: Update Checkpoints in Org messages are printed. I've attached a screenshot below!

Screen Shot 2021-07-21 at 12 57 58 PM

Awesome thank you for reminding me! Is it possible for us to exit out of the Uploading Checkpoints process once we discover that there aren't any checkpoints so we can avoid printing the rest of the messages as well?

@violetyao
Copy link
Contributor Author

Great point! I agree that the messages are confusing if there is no checkpoint available. I've added a hasOneOrMoreActiveCheckpoints function and attached a screenshot of the outputs if there is no checkpoint available.
Screen Shot 2021-07-21 at 3 08 59 PM

Let me know what you think! If the change looks good to you, let's have Sonal review the no_enabled_checkpoints message wording.

@AnanyaJha
Copy link
Collaborator

Great point! I agree that the messages are confusing if there is no checkpoint available. I've added a hasOneOrMoreActiveCheckpoints function and attached a screenshot of the outputs if there is no checkpoint available.
Screen Shot 2021-07-21 at 3 08 59 PM

Let me know what you think! If the change looks good to you, let's have Sonal review the no_enabled_checkpoints message wording.

Do you think the user needs to be notified that they don't have any checkpoints at all? @jag-sfdc thoughts? If anything, I think it should be a warning modal, not an error one.

@violetyao
Copy link
Contributor Author

Great point! I agree that the messages are confusing if there is no checkpoint available. I've added a hasOneOrMoreActiveCheckpoints function and attached a screenshot of the outputs if there is no checkpoint available.
Screen Shot 2021-07-21 at 3 08 59 PM
Let me know what you think! If the change looks good to you, let's have Sonal review the no_enabled_checkpoints message wording.

Do you think the user needs to be notified that they don't have any checkpoints at all? @jag-sfdc thoughts? If anything, I think it should be a warning modal, not an error one.

I'm leaning towards still giving users warnings if they don't have any checkpoints at all. It might be confusing to users when they try todebug & run and expect that tests stop at checkpoints. I agree that a warning modal is better than an error. Let us know what you think Jag!

@AnanyaJha AnanyaJha self-requested a review July 27, 2021 05:49
@violetyao violetyao merged commit 7ac0998 into develop Jul 28, 2021
@violetyao violetyao deleted the violet/one-step-debugger-upload-checkpoints branch July 28, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants