@W-11494080@ Fix survey request formatting issue #773
Conversation
rmohan20
changed the title
| /** | ||
| * Common steps that should be run before every command | ||
| */ | ||
| protected runCommonSteps(): void { |
There was a problem hiding this comment.
A place to put common steps so that we don't have to modify each command every time we change things.
There was a problem hiding this comment.
This being its own method feels excessive. You could just do these things in run() before invoking runInternal()
There was a problem hiding this comment.
runCommonSteps() behaves as a hook that would continue to keep run() more readable as the common steps expand.
Also, we may have a subgroup of commands that require to do a variation of the common steps and override it to their convenience. For example, in an abstract class like ScannerRunCommand.
In near future, if we decide to remove the survey link, I'd still recommend leaving the hook with a marker comment on what can possibly be added there.
rmohan20
force-pushed
the
rm/jsonFormatIssue-3.x
branch
from
3d19af0 to
51284f4
Compare
| /** | ||
| * Common steps that should be run before every command | ||
| */ | ||
| protected runCommonSteps(): void { |
There was a problem hiding this comment.
This being its own method feels excessive. You could just do these things in run() before invoking runInternal()
| expect(ctxJson.result.length).to.equal(0, 'Should be no results'); | ||
| expect(ctxJson.warnings.length).to.equal(1, 'Should be one warning'); | ||
| expect(ctxJson.warnings[0]).to.equal(formattedWarning, 'Warning message should match'); | ||
| expect(ctxJson.warnings.length).to.equal(2, 'Should be one warning'); |
There was a problem hiding this comment.
The failure text says "one" instead of "two".
No description provided.