Enable Checkpoints by default #664
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #664 +/- ##
==========================================
Coverage ? 74.87%
==========================================
Files ? 158
Lines ? 6431
Branches ? 1009
==========================================
Hits ? 4815
Misses ? 1358
Partials ? 258Continue to review full report at Codecov.
|
| @@ -218,7 +218,7 @@ | |||
| "properties": { | |||
| "salesforcedx-vscode-apex-replay-debugger-checkpoints.enabled": { | |||
There was a problem hiding this comment.
Won't it be better to just remove this whole properties? That's what we did before after we are ready to GA something that was guarded.
Or, do you
- Think that is too risky?
- Envision that you might want to actually have some people be able to toggle between the two?
There was a problem hiding this comment.
@vazexqi we (the team) had talked about this in standup earlier this week. We decided that leaving the flag in, for the moment, was the way we wanted to handle this right now. We are ultimately going to remove this but with the current timeline this was the safest change.
|
Updated the .appveyor.yml file and the package.json to prevent UI system-tests from running in appveyor. The reason for this is the XPath for UI elements changed in VS Code 1.28 and the tests are going to need to be updated. |
| @@ -31,8 +31,6 @@ | |||
| "test:integration": "lerna run test:integration --concurrency 1 --stream", | |||
| "test:without-system-tests": | |||
| "lerna exec --ignore system-tests --concurrency 1 --stream --bail=false -- npm run test", | |||
| "test:system-tests": | |||
There was a problem hiding this comment.
Do we need to remove this? It seems benign and safer to leave this in and just use the commented out technique up there.
There was a problem hiding this comment.
@vazexqi In https://github.com/forcedotcom/salesforcedx-vscode/pull/559/files, I removed this line because I thought that the test script would run all scripts in the package.json that began with "test:*", and I wanted to prevent the system-tests from being run. However, I see that in the .appveyor.yml, line 43 runs test:without-system-tests so I'm a bit confused. If the autobuild is already running the script that ignores system tests, then is it the npm run coverage:system-tests that invokes the system tests?
There was a problem hiding this comment.
I'm also not sure if comments in a package.json are a thing. From what I've read there's no comment syntax for the entries that isn't hacky. Removal and re-addition seem like the way to go here.
There was a problem hiding this comment.
@vazexqi things are ready but I'm not going to submit until you say so.
There was a problem hiding this comment.
If the autobuild is already running the script that ignores system tests, then is it the npm run coverage:system-tests that invokes the system tests?
Yes, it is coverage:system-tests that invokes the system tests. This can be seen at https://github.com/forcedotcom/salesforcedx-vscode/blob/develop/package.json#L37 where it will invoke the system tests.
@JimSuplizio
In the interest of time, let's proceed with what we have and restore it back in "W-5507653 - User Story - Re-enable UI system-tests in AppVeyor".
What does this PR do?
Flips the salesforcedx-vscode-apex-replay-debugger-checkpoints.enabled boolean from false to true, enabling checkpoints by default. It was decided for GA that flipping the boolean was enough. A second work item will be created for the removal of the setting and conditional code entirely but those changes would be made at a later date.
What issues does this PR fix or reference?
@W-5491026@