Skip to content

CHANGE @W-17701097@ Logfile now communicated at start. #1745

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.

Sign up for GitHub

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

Merged
jfeingold35 merged 2 commits into from
Feb 18, 2025
Merged

CHANGE @W-17701097@ Logfile now communicated at start. #1745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e035132
CHANGE @W-17701097@ Logfile now communicated at start.
jfeingold35 Feb 18, 2025
b64c973
@W-17701097@ Finalizing new message post-approval
jfeingold35 Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions messages/action-summary-viewer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# common.streaming-logs-to

Streaming logs in real time to:

# common.summary-header

Summary
Expand Down
3 changes: 2 additions & 1 deletion src/lib/actions/ConfigAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export class ConfigAction {

// We always add a Logger Listener to the appropriate listeners list, because we should always be logging.
const logFileWriter: LogFileWriter = await LogFileWriter.fromConfig(userConfig);
this.dependencies.actionSummaryViewer.viewPreExecutionSummary(logFileWriter.getLogDestination());
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

This is the functional change to this Action. I added it as early in the method as I could (i.e., immediately after the log file writer is instantiated) to minimize the possibility of it being preempted by an error.
If we want, though, we could easily move it further down and use it in the future to log additional information about, for example, the engines that are added. I'm flexible on this.

const logEventLogger: LogEventLogger = new LogEventLogger(logFileWriter);
this.dependencies.logEventListeners.push(logEventLogger);

Expand Down Expand Up @@ -117,7 +118,7 @@ export class ConfigAction {
this.dependencies.viewer.view(configModel);
}

this.dependencies.actionSummaryViewer.view(logFileWriter.getLogDestination(), fileWritten ? input['output-file'] : undefined);
this.dependencies.actionSummaryViewer.viewPostExecutionSummary(logFileWriter.getLogDestination(), fileWritten ? input['output-file'] : undefined);
return Promise.resolve();
}

Expand Down
3 changes: 2 additions & 1 deletion src/lib/actions/RulesAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export class RulesAction {
public async execute(input: RulesInput): Promise<void> {
const config: CodeAnalyzerConfig = this.dependencies.configFactory.create(input['config-file']);
const logWriter: LogFileWriter = await LogFileWriter.fromConfig(config);
this.dependencies.actionSummaryViewer.viewPreExecutionSummary(logWriter.getLogDestination());
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

This is the functional change to this Action. I added it as early in the method as I could (i.e., immediately after the log file writer is instantiated) to minimize the possibility of it being preempted by an error.
If we want, though, we could easily move it further down and use it in the future to log additional information about, for example, the engines that are added. I'm flexible on this.

// We always add a Logger Listener to the appropriate listeners list, because we should Always Be Logging.
this.dependencies.logEventListeners.push(new LogEventLogger(logWriter));
const core: CodeAnalyzer = new CodeAnalyzer(config);
Expand Down Expand Up @@ -60,7 +61,7 @@ export class RulesAction {
const rules: Rule[] = core.getEngineNames().flatMap(name => ruleSelection.getRulesFor(name));

this.dependencies.viewer.view(rules);
this.dependencies.actionSummaryViewer.view(ruleSelection, logWriter.getLogDestination());
this.dependencies.actionSummaryViewer.viewPostExecutionSummary(ruleSelection, logWriter.getLogDestination());
}

public static createAction(dependencies: RulesDependencies): RulesAction {
Expand Down
3 changes: 2 additions & 1 deletion src/lib/actions/RunAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export class RunAction {
public async execute(input: RunInput): Promise<void> {
const config: CodeAnalyzerConfig = this.dependencies.configFactory.create(input['config-file']);
const logWriter: LogFileWriter = await LogFileWriter.fromConfig(config);
this.dependencies.actionSummaryViewer.viewPreExecutionSummary(logWriter.getLogDestination());
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

This is the functional change to this Action. I added it as early in the method as I could (i.e., immediately after the log file writer is instantiated) to minimize the possibility of it being preempted by an error.
If we want, though, we could easily move it further down and use it in the future to log additional information about, for example, the engines that are added. I'm flexible on this.

// We always add a Logger Listener to the appropriate listeners list, because we should Always Be Logging.
this.dependencies.logEventListeners.push(new LogEventLogger(logWriter));
const core: CodeAnalyzer = new CodeAnalyzer(config);
Expand Down Expand Up @@ -80,7 +81,7 @@ export class RunAction {
this.dependencies.logEventListeners.forEach(listener => listener.stopListening());
this.dependencies.writer.write(results);
this.dependencies.resultsViewer.view(results);
this.dependencies.actionSummaryViewer.view(results, logWriter.getLogDestination(), input['output-file']);
this.dependencies.actionSummaryViewer.viewPostExecutionSummary(results, logWriter.getLogDestination(), input['output-file']);

const thresholdValue = input['severity-threshold'];
if (thresholdValue) {
Expand Down
17 changes: 14 additions & 3 deletions src/lib/viewers/ActionSummaryViewer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ abstract class AbstractActionSummaryViewer {
this.display = display;
}

public viewPreExecutionSummary(logFile: string): void {
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

The Display object isn't directly accessible to the Action layer, so I opted to add the pre-execution output as another capability of the ActionSummaryViewer family instead of giving the Actions a new Dependency.

// Start with separator to cleanly break from anything that's already been logged.
this.displayLineSeparator();
this.display.displayLog(getMessage(BundleName.ActionSummaryViewer, 'common.streaming-logs-to'));
this.display.displayLog(indent(logFile));
// End with a separator to cleanly break with anything that comes next.
this.displayLineSeparator();
}

protected displaySummaryHeader(): void {
this.display.displayLog(toStyledHeader(getMessage(BundleName.ActionSummaryViewer, 'common.summary-header')));
}
Expand All @@ -29,7 +38,9 @@ export class ConfigActionSummaryViewer extends AbstractActionSummaryViewer {
super(display);
}

public view(logFile: string, outfile?: string): void {
public viewPostExecutionSummary(logFile: string, outfile?: string): void {
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

The concrete summary viewers' view method was renamed to viewPostExecutionSummary to distinguish it from the net-new viewPreExecutionSummary.

// Start with separator to cleanly break from anything that's already been logged.
this.displayLineSeparator();
Copy link
Contributor Author

@jfeingold35 jfeingold35 Feb 18, 2025

Choose a reason for hiding this comment

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

This was missing the leading newline that all the other summary viewers have, so I went ahead and added it.

this.displaySummaryHeader();
this.displayLineSeparator();

Expand All @@ -52,7 +63,7 @@ export class RulesActionSummaryViewer extends AbstractActionSummaryViewer {
super(display);
}

public view(ruleSelection: RuleSelection, logFile: string): void {
public viewPostExecutionSummary(ruleSelection: RuleSelection, logFile: string): void {
// Start with separator to cleanly break from anything that's already been logged.
this.displayLineSeparator();
this.displaySummaryHeader();
Expand Down Expand Up @@ -82,7 +93,7 @@ export class RunActionSummaryViewer extends AbstractActionSummaryViewer {
super(display);
}

public view(results: RunResults, logFile: string, outfiles: string[]): void {
public viewPostExecutionSummary(results: RunResults, logFile: string, outfiles: string[]): void {
// Start with separator to cleanly break from anything that's already been logged.
this.displayLineSeparator();
this.displaySummaryHeader();
Expand Down
1 change: 1 addition & 0 deletions ...n-files/lib/actions/ConfigAction.test.ts/action-summaries/no-outfile-created.txt.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

=== Summary

Additional log information written to:
1 change: 1 addition & 0 deletions ...ison-files/lib/actions/ConfigAction.test.ts/action-summaries/outfile-created.txt.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

=== Summary

Configuration written to:
Expand Down
2 changes: 2 additions & 0 deletions ...iles/lib/actions/ConfigAction.test.ts/action-summaries/pre-execution-summary.txt.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

Streaming logs in real time to:
2 changes: 2 additions & 0 deletions ...files/lib/actions/RulesAction.test.ts/action-summaries/pre-execution-summary.txt.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

Streaming logs in real time to:
2 changes: 2 additions & 0 deletions ...n-files/lib/actions/RunAction.test.ts/action-summaries/pre-execution-summary.txt.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

Streaming logs in real time to:
8 changes: 6 additions & 2 deletions test/lib/actions/ConfigAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ describe('ConfigAction tests', () => {
.map(e => e.data)
.join('\n'));

const preExecutionGoldfileContents: string = await readGoldFile(path.join(PATH_TO_COMPARISON_DIR, 'action-summaries', 'pre-execution-summary.txt.goldfile'));
expect(displayedLogEvents).toContain(preExecutionGoldfileContents);
const goldfileContents: string = await readGoldFile(path.join(PATH_TO_COMPARISON_DIR, 'action-summaries', 'outfile-created.txt.goldfile'));
expect(displayedLogEvents).toContain(goldfileContents);
});
Expand Down Expand Up @@ -496,6 +498,8 @@ describe('ConfigAction tests', () => {
.map(e => e.data)
.join('\n'));

const preExecutionGoldfileContents: string = await readGoldFile(path.join(PATH_TO_COMPARISON_DIR, 'action-summaries', 'pre-execution-summary.txt.goldfile'));
expect(displayedLogEvents).toContain(preExecutionGoldfileContents);
const goldfileContents: string = await readGoldFile(path.join(PATH_TO_COMPARISON_DIR, 'action-summaries', 'no-outfile-created.txt.goldfile'));
expect(displayedLogEvents).toContain(goldfileContents);
});
Expand All @@ -519,8 +523,8 @@ describe('ConfigAction tests', () => {

// ==== OUTPUT PROCESSING ====
const displayEvents = spyDisplay.getDisplayEvents();
expect(displayEvents[0].type).toEqual(DisplayEventType.LOG);
return ansis.strip(displayEvents[0].data);
expect(displayEvents[4].type).toEqual(DisplayEventType.LOG);
return ansis.strip(displayEvents[4].data);
}
});

Expand Down
3 changes: 3 additions & 0 deletions test/lib/actions/RulesAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ describe('RulesAction tests', () => {
{quantifier: 'no', expectation: 'Summary indicates absence of rules', selector: 'NonsensicalTag', goldfile: 'no-rules.txt.goldfile'},
{quantifier: 'some', expectation: 'Summary provides breakdown by engine', selector: 'Recommended', goldfile: 'some-rules.txt.goldfile'}
])('When $quantifier rules are returned, $expectation', async ({selector, goldfile}) => {
const preExecutionGoldfilePath: string = path.join(PATH_TO_GOLDFILES, 'action-summaries', 'pre-execution-summary.txt.goldfile');
const goldfilePath: string = path.join(PATH_TO_GOLDFILES, 'action-summaries', goldfile);
const spyDisplay: SpyDisplay = new SpyDisplay();
const actionSummaryViewer: RulesActionSummaryViewer = new RulesActionSummaryViewer(spyDisplay);
Expand All @@ -183,6 +184,8 @@ describe('RulesAction tests', () => {
.filter(e => e.type === DisplayEventType.LOG)
.map(e => e.data)
.join('\n'));
const preExecutionGoldfileContents: string = await fsp.readFile(preExecutionGoldfilePath, 'utf-8');
expect(displayedLogEvents).toContain(preExecutionGoldfileContents);

const goldfileContents: string = await fsp.readFile(goldfilePath, 'utf-8');
expect(displayedLogEvents).toContain(goldfileContents);
Expand Down
4 changes: 4 additions & 0 deletions test/lib/actions/RunAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,14 @@ describe('RunAction tests', () => {
const actualTargetFiles = engine1.runRulesCallHistory[0].runOptions.workspace.getFilesAndFolders();
expect(actualTargetFiles).toEqual([path.resolve('.')]);
// Verify that the summary output matches the expectation.
const preExecutionGoldfileContents: string = await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', 'pre-execution-summary.txt.goldfile'), 'utf-8');
const goldfileContents: string = await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', goldfile), 'utf-8');
const displayEvents = spyDisplay.getDisplayEvents();
const displayedLogEvents = ansis.strip(displayEvents
.filter(e => e.type === DisplayEventType.LOG)
.map(e => e.data)
.join('\n'));
expect(displayedLogEvents).toContain(preExecutionGoldfileContents);
expect(displayedLogEvents).toContain(goldfileContents);
});

Expand Down Expand Up @@ -337,6 +339,7 @@ describe('RunAction tests', () => {
const actualTargetFiles = engine1.runRulesCallHistory[0].runOptions.workspace.getFilesAndFolders();
expect(actualTargetFiles).toEqual([path.resolve('.')]);
// Verify that the summary output matches the expectation.
const preExecutionGoldfileContents: string = await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', 'pre-execution-summary.txt.goldfile'), 'utf-8');
const goldfileContents: string = (await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', 'some-outfiles.txt.goldfile'), 'utf-8'))
.replace(`{{PATH_TO_FILE1}}`, outfilePath1)
.replace(`{{PATH_TO_FILE2}}`, outfilePath2);
Expand All @@ -345,6 +348,7 @@ describe('RunAction tests', () => {
.filter(e => e.type === DisplayEventType.LOG)
.map(e => e.data)
.join('\n'));
expect(displayedLogEvents).toContain(preExecutionGoldfileContents);
expect(displayedLogEvents).toContain(goldfileContents);
});
});
Expand Down