Skip to content

Commit

Permalink
Some small cleanup changes pulled out of the quiet logger PR (#838)
Browse files Browse the repository at this point in the history 
* Some small changes to prepare for the quiet logger.

Adds some more events, logs them more consistently, puts the logger in control of how it's wrapped when in watch mode.

* A few more minor changes

* Add an additional uninteresting event

* Separate out tests that don't require network access.

* Address review comments
  • Loading branch information
@rictic
rictic authored Aug 30, 2023
1 parent 6ecf656 commit a7c4a1c
Show file tree
Hide file tree
Showing 14 changed files with 207 additions and 87 deletions.
2 changes: 1 addition & 1 deletion .vscode/settings.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
"javascript.preferences.importModuleSpecifierEnding": "js",
"typescript.preferences.importModuleSpecifierEnding": "js",
"editor.defaultFormatter": "esbenp.prettier-vscode",
"typescript.tsdk": "node_modules\\typescript\\lib",
"typescript.tsdk": "node_modules/typescript/lib",
"typescript.enablePromptUseWorkspaceTsdk": true
}
9 changes: 8 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"format:check": "prettier . -c",
"test": "wireit",
"test:headless": "wireit",
"test:local": "wireit",
"test:analysis": "wireit",
"test:basic": "wireit",
"test:cache-github": "wireit",
Expand Down Expand Up @@ -75,10 +76,16 @@
]
},
"test:headless": {
"dependencies": [
"test:cache-github",
"test:local"
]
},
"test:local": {
"#comment": "Run tests that don't require network access.",
"dependencies": [
"test:analysis",
"test:basic",
"test:cache-github",
"test:cache-local",
"test:clean",
"test:cli-options",
Expand Down
26 changes: 25 additions & 1 deletion src/analyzer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type {
ScriptReferenceString,
} from './config.js';
import type {Agent} from './cli-options.js';
import {Logger} from './logging/logger.js';

export interface AnalyzeResult {
config: Result<ScriptConfig, Failure[]>;
Expand Down Expand Up @@ -147,9 +148,11 @@ export class Analyzer {
private readonly _ongoingWorkPromises: Array<Promise<undefined>> = [];
private readonly _relevantConfigFilePaths = new Set<string>();
private readonly _agent: Agent;
private readonly _logger: Logger | undefined;

constructor(agent: Agent, filesystem?: FileSystem) {
constructor(agent: Agent, logger?: Logger, filesystem?: FileSystem) {
this._agent = agent;
this._logger = logger;
this._packageJsonReader = new CachingPackageJsonReader(filesystem);
}

Expand Down Expand Up @@ -200,6 +203,27 @@ export class Analyzer {
async analyze(
root: ScriptReference,
extraArgs: string[] | undefined
): Promise<AnalyzeResult> {
this._logger?.log({
type: 'info',
detail: 'analysis-started',
script: root,
});
const analyzeResult = await this._actuallyAnalyze(root, extraArgs);
this._logger?.log({
type: 'info',
detail: 'analysis-completed',
script: root,
rootScriptConfig: analyzeResult.config.ok
? analyzeResult.config.value
: undefined,
});
return analyzeResult;
}

private async _actuallyAnalyze(
root: ScriptReference,
extraArgs: string[] | undefined
): Promise<AnalyzeResult> {
// We do 2 walks through the dependency graph:
//
Expand Down
8 changes: 4 additions & 4 deletions src/caching/github-actions-cache.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class GitHubActionsCache implements Cache {
this._logger.log({
script,
type: 'info',
detail: 'generic',
detail: 'cache-info',
message:
`Output was too big to be cached: ` +
`${Math.round(tarballBytes / GB)}GB > ` +
Expand Down Expand Up @@ -364,7 +364,7 @@ export class GitHubActionsCache implements Cache {
this._logger.log({
script,
type: 'info',
detail: 'generic',
detail: 'cache-info',
message:
`Connection error from GitHub Actions service, caching disabled. ` +
'Detail: ' +
Expand All @@ -381,7 +381,7 @@ export class GitHubActionsCache implements Cache {
this._logger.log({
script,
type: 'info',
detail: 'generic',
detail: 'cache-info',
message: `Hit GitHub Actions cache rate limit, caching disabled.`,
});
}
Expand All @@ -392,7 +392,7 @@ export class GitHubActionsCache implements Cache {
this._logger.log({
script,
type: 'info',
detail: 'generic',
detail: 'cache-info',
message: `GitHub Actions service is unavailable, caching disabled.`,
});
}
Expand Down
63 changes: 52 additions & 11 deletions src/event.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ interface EventBase<T extends PackageReference = ScriptReference> {
// Success events
// -------------------------------

type Success = ExitZero | NoCommand | Fresh | Cached;
/**
* A script finished successfully.
*/
export type Success = ExitZero | NoCommand | Fresh | Cached;

interface SuccessBase<T extends PackageReference = ScriptReference>
extends EventBase<T> {
Expand Down Expand Up @@ -70,6 +73,9 @@ export interface Cached extends SuccessBase {
// Failure events
// -------------------------------

/**
* A problem was encountered.
*/
export type Failure =
| ExitNonZero
| ExitSignal
Expand Down Expand Up @@ -313,7 +319,10 @@ export interface UnknownErrorThrown extends ErrorBase {
// Output events
// -------------------------------

type Output = Stdout | Stderr;
/**
* A script emitted output.
*/
export type Output = Stdout | Stderr;

interface OutputBase extends EventBase<ScriptConfig> {
type: 'output';
Expand All @@ -338,15 +347,21 @@ export interface Stderr extends OutputBase {
// Informational events
// -------------------------------

type Info =
/**
* Something happened, neither success nor failure, though often progress.
*/
export type Info =
| ScriptRunning
| ScriptLocked
| OutputModified
| WatchRunStart
| WatchRunEnd
| ServiceStarted
| ServiceProcessStarted
| ServiceReady
| ServiceStopped
| GenericInfo;
| AnalysisStarted
| AnalysisCompleted
| CacheInfo;

interface InfoBase<T extends PackageReference = ScriptReference>
extends EventBase<T> {
Expand Down Expand Up @@ -377,6 +392,24 @@ export interface OutputModified extends InfoBase {
detail: 'output-modified';
}

/**
* The analysis phase for a run has begun, where we load package.json
* files, analyze their wireit configs, and build the dependency graph.
*/
export interface AnalysisStarted extends InfoBase {
detail: 'analysis-started';
}

/**
* The analysis phase for a run has completed. If successful, we have a
* rootScriptConfig with a full dependency graph. If unsuccessful, it is
* undefined, and there will be a Failure event with more information.
*/
export interface AnalysisCompleted extends InfoBase {
detail: 'analysis-completed';
rootScriptConfig: undefined | ScriptConfig;
}

/**
* A watch mode iteration started.
*/
Expand All @@ -392,10 +425,18 @@ export interface WatchRunEnd extends InfoBase {
}

/**
* A service started running.
* A service process started running.
*/
export interface ServiceProcessStarted extends InfoBase {
detail: 'service-process-started';
}

/**
* A service started running and if it has a readyWhen condition,
* that condition is met.
*/
export interface ServiceStarted extends InfoBase {
detail: 'service-started';
export interface ServiceReady extends InfoBase {
detail: 'service-ready';
}

/**
Expand All @@ -406,9 +447,9 @@ export interface ServiceStopped extends InfoBase {
}

/**
* A generic info event.
* An advisory event about caching.
*/
export interface GenericInfo extends InfoBase {
detail: 'generic';
export interface CacheInfo extends InfoBase {
detail: 'cache-info';
message: string;
}
28 changes: 16 additions & 12 deletions src/execution/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,15 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
entireExecutionAborted,
adoptee,
};
// Doing this here ensures that we always log when the
// service stops, no matter how that happens.
void this._terminated.promise.then(() => {
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-stopped',
});
});
}

/**
Expand Down Expand Up @@ -818,6 +827,11 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
private _onChildStarted() {
switch (this._state.id) {
case 'starting': {
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-process-started',
});
if (this._state.readyMonitor !== undefined) {
this._state = {
id: 'readying',
Expand All @@ -838,7 +852,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-started',
detail: 'service-ready',
});
this._state = {
id: 'started',
Expand Down Expand Up @@ -879,7 +893,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-started',
detail: 'service-ready',
});
this._state = {
id: 'started',
Expand Down Expand Up @@ -914,11 +928,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
switch (this._state.id) {
case 'stopping': {
this._enterStoppedState();
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-stopped',
});
return;
}
case 'readying': {
Expand All @@ -944,11 +953,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
return;
}
case 'failing': {
this._logger.log({
script: this._config,
type: 'info',
detail: 'service-stopped',
});
this._enterFailedState(this._state.failure);
return;
}
Expand Down
6 changes: 3 additions & 3 deletions src/ide.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class IdeAnalyzer {
private _analyzer;
constructor() {
this._overlayFs = new OverlayFilesystem();
this._analyzer = new Analyzer('npm', this._overlayFs);
this._analyzer = new Analyzer('npm', undefined, this._overlayFs);
}

/**
Expand All @@ -73,15 +73,15 @@ export class IdeAnalyzer {
*/
setOpenFileContents(path: string, contents: string): void {
this._overlayFs.overlay.set(path, contents);
this._analyzer = new Analyzer('npm', this._overlayFs);
this._analyzer = new Analyzer('npm', undefined, this._overlayFs);
}

/**
* Removes a file from the set of open files.
*/
closeFile(path: string): void {
this._overlayFs.overlay.delete(path);
this._analyzer = new Analyzer('npm', this._overlayFs);
this._analyzer = new Analyzer('npm', undefined, this._overlayFs);
}

get openFiles(): Iterable<string> {
Expand Down
Loading

0 comments on commit a7c4a1c

Please sign in to comment.