Report all new CTQ errors on Slack #141
Comments
|
@zepumph said in the 5/19/2022 developer meeting that #140 should be fixed soon, so there is no need to explicitly filter it out. As for the 2nd proposal (alerting us when things change in the failure modes), @jonathanolson says that there is a reasonably quick way to try this out, which is to diff the failures and send a notification on changes (i.e. differences exist). He will implement this. We would like to not get notifications when errors are fixed until they are all fixed, i.e. all lint is now passing. |
|
I made progress on this issue tonight. So far, I have just been experimenting with lint + tsc errors, as adding in transpiling and fuzzing makes for longer iterations and additional noise with memory stuff. So more work may be needed so support bringing those back, but I think I have a nice UX working for tsc and lint errors. Here is a log of a trial run, with my "commit" actions noted during each "checking stale" stage. When any messages would be sent to CT is noted in the logs. A patch of my working copy changes is down below the log output. Index: js/server/QuickServer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/QuickServer.js b/js/server/QuickServer.js
--- a/js/server/QuickServer.js (revision e11b7af15b8976ab74a2db6f66a8140128fa4616)
+++ b/js/server/QuickServer.js (date 1653368691952)
@@ -16,8 +16,8 @@
const gruntCommand = require( '../../../perennial/js/common/gruntCommand' );
const isStale = require( '../../../perennial/js/common/isStale' );
const npmUpdate = require( '../../../perennial/js/common/npmUpdate' );
-const puppeteerLoad = require( '../../../perennial/js/common/puppeteerLoad' );
-const withServer = require( '../../../perennial/js/common/withServer' );
+// const puppeteerLoad = require( '../../../perennial/js/common/puppeteerLoad' );
+// const withServer = require( '../../../perennial/js/common/withServer' );
const assert = require( 'assert' );
const http = require( 'http' );
const _ = require( 'lodash' ); // eslint-disable-line
@@ -45,7 +45,7 @@
};
// @public {*}
- this.reportState = {};
+ this.testingState = {};
// @public {string} - root of your GitHub working copy, relative to the name of the directory that the
// currently-executing script resides in
@@ -53,6 +53,9 @@
// @public {boolean} - whether we are in testing mode. if true, tests are continuously forced to run
this.isTestMode = options.isTestMode;
+
+ // @public {string[]} - errors found in any given loop
+ this.errorMessages = [];
}
/**
@@ -120,110 +123,88 @@
winston.info( 'QuickServer: tsc' );
const tscResult = await execute( '../../node_modules/typescript/bin/tsc', [], `${this.rootDir}/chipper/tsconfig/all`, { errors: 'resolve' } );
- winston.info( 'QuickServer: transpiling' );
- const transpileResult = await execute( 'node', [ 'js/scripts/transpile.js' ], `${this.rootDir}/chipper`, { errors: 'resolve' } );
+ // winston.info( 'QuickServer: transpiling' );
+ // const transpileResult = await execute( 'node', [ 'js/scripts/transpile.js' ], `${this.rootDir}/chipper`, { errors: 'resolve' } );
- winston.info( 'QuickServer: sim fuzz' );
- let simFuzz = null;
- try {
- await withServer( async port => {
- const url = `http://localhost:${port}/natural-selection/natural-selection_en.html?brand=phet&ea&debugger&fuzz`;
- const error = await puppeteerLoad( url, {
- waitAfterLoad: 10000,
- allowedTimeToLoad: 120000,
- puppeteerTimeout: 120000
- } );
- if ( error ) {
- simFuzz = error;
- }
- } );
- }
- catch( e ) {
- simFuzz = e;
- }
-
- winston.info( 'QuickServer: studio fuzz' );
- let studioFuzz = null;
- try {
- await withServer( async port => {
- const url = `http://localhost:${port}/studio/index.html?sim=states-of-matter&phetioElementsDisplay=all&fuzz`;
- const error = await puppeteerLoad( url, {
- waitAfterLoad: 10000,
- allowedTimeToLoad: 120000,
- puppeteerTimeout: 120000
- } );
- if ( error ) {
- studioFuzz = error;
- }
- } );
- }
- catch( e ) {
- studioFuzz = e;
- }
+ // winston.info( 'QuickServer: sim fuzz' );
+ // let simFuzz = null;
+ // try {
+ // await withServer( async port => {
+ // const url = `http://localhost:${port}/natural-selection/natural-selection_en.html?brand=phet&ea&debugger&fuzz`;
+ // const error = await puppeteerLoad( url, {
+ // waitAfterLoad: 10000,
+ // allowedTimeToLoad: 120000,
+ // puppeteerTimeout: 120000
+ // } );
+ // if ( error ) {
+ // simFuzz = error;
+ // }
+ // } );
+ // }
+ // catch( e ) {
+ // simFuzz = e;
+ // }
+ //
+ // winston.info( 'QuickServer: studio fuzz' );
+ // let studioFuzz = null;
+ // try {
+ // await withServer( async port => {
+ // const url = `http://localhost:${port}/studio/index.html?sim=states-of-matter&phetioElementsDisplay=all&fuzz`;
+ // const error = await puppeteerLoad( url, {
+ // waitAfterLoad: 10000,
+ // allowedTimeToLoad: 120000,
+ // puppeteerTimeout: 120000
+ // } );
+ // if ( error ) {
+ // studioFuzz = error;
+ // }
+ // } );
+ // }
+ // catch( e ) {
+ // studioFuzz = e;
+ // }
const executeResultToOutput = result => {
return {
passed: result.code === 0,
- message: `code: ${result.code}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`
+ // TODO: Can the message just be stdout? Why are the other values needed? Code is just a number, and
+ // stderr seems to always be blank
+ message: `code: ${result.code}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`,
+ stdout: result.stdout
};
};
- const fuzzResultToOutput = result => {
- if ( result === null ) {
- return { passed: true, message: '' };
- }
- else {
- return { passed: false, message: '' + result };
- }
- };
+ // const fuzzResultToOutput = result => {
+ // if ( result === null ) {
+ // return { passed: true, message: '' };
+ // }
+ // else {
+ // return { passed: false, message: '' + result };
+ // }
+ // };
// This would take up too much space
- transpileResult.stdout = '';
+ // transpileResult.stdout = '';
- this.reportState = {
+ this.testingState = {
lint: executeResultToOutput( lintResult ),
tsc: executeResultToOutput( tscResult ),
- transpile: executeResultToOutput( transpileResult ),
- simFuzz: fuzzResultToOutput( simFuzz ),
- studioFuzz: fuzzResultToOutput( studioFuzz ),
+ // transpile: executeResultToOutput( transpileResult ),
+ // simFuzz: fuzzResultToOutput( simFuzz ),
+ // studioFuzz: fuzzResultToOutput( studioFuzz ),
shas: shas,
timestamp: timestamp
};
try {
- const broken = !this.reportState.lint.passed ||
- !this.reportState.tsc.passed ||
- !this.reportState.transpile.passed ||
- !this.reportState.simFuzz.passed ||
- !this.reportState.studioFuzz.passed;
-
- if ( lastBroken !== broken ) {
-
- if ( broken ) {
- winston.info( 'sending broken CT message' );
- let message = 'Quick CT failing';
-
- const check = ( result, name ) => {
- if ( !result.passed ) {
- message += `\n${name}:\n\`\`\`\n${result.message}\n\`\`\`\n`;
- }
- };
+ const broken = !this.testingState.lint.passed ||
+ !this.testingState.tsc.passed;
+ // !this.testingState.transpile.passed ||
+ // !this.testingState.simFuzz.passed ||
+ // !this.testingState.studioFuzz.passed;
- check( this.reportState.lint, 'lint' );
- check( this.reportState.tsc, 'tsc' );
- check( this.reportState.transpile, 'transpile' );
- check( this.reportState.simFuzz, 'simFuzz' );
- check( this.reportState.studioFuzz, 'studioFuzz' );
- winston.info( message );
-
- await sendSlackMessage( message );
- }
- else {
- winston.info( 'sending passing CT message' );
- await sendSlackMessage( 'Quick CT passing' );
- }
+ await this.reportErrorStatus( broken, lastBroken );
- lastBroken = broken;
- }
+ lastBroken = broken;
}
catch( e ) {
winston.info( `Slack error: ${e}` );
@@ -253,7 +234,7 @@
if ( requestInfo.pathname === '/quickserver/status' ) {
res.writeHead( 200, jsonHeaders );
- res.end( JSON.stringify( this.reportState, null, 2 ) );
+ res.end( JSON.stringify( this.testingState, null, 2 ) );
}
}
catch( e ) {
@@ -263,6 +244,63 @@
winston.info( `QuickServer: running on port ${port}` );
}
+
+ /**
+ * Checks the error messages and reports the current status to the logs and Slack.
+ *
+ * @param {boolean} broken
+ * @param {boolean} lastBroken
+ * @private
+ */
+ async reportErrorStatus( broken, lastBroken ) {
+ if ( lastBroken !== broken && !broken ) {
+ this.errorMessages.length = 0;
+ winston.info( 'broken -> passing, sending passing CT message' );
+ !this.isTestMode && await sendSlackMessage( 'Quick CT passing' );
+ }
+ else if ( broken ) {
+ let message = '';
+ let previousFailureFound = false;
+
+ const check = ( result, name ) => {
+ if ( !result.passed && !this.errorMessages.includes( result.stdout ) ) {
+ this.errorMessages.push( result.stdout );
+ message += `\n${name}:\n\`\`\`\n${result.message}\n\`\`\`\n`;
+ }
+ else if ( !result.passed ) {
+ previousFailureFound = true;
+ }
+ };
+
+ check( this.testingState.lint, 'lint' );
+ check( this.testingState.tsc, 'tsc' );
+ // check( this.testingState.transpile, 'transpile' );
+ // check( this.testingState.simFuzz, 'simFuzz' );
+ // check( this.testingState.studioFuzz, 'studioFuzz' );
+
+ if ( message.length > 0 ) {
+
+ if ( previousFailureFound ) {
+ winston.info( 'broken -> more broken, sending additional broken CT message' );
+ message = 'Quick CT additional failure:' + message;
+ }
+ else {
+ winston.info( 'passing -> broken, sending broken CT message' );
+ message = 'Quick CT failing:' + message;
+ }
+
+ winston.info( message );
+ !this.isTestMode && await sendSlackMessage( message );
+ }
+ else {
+ assert && assert( previousFailureFound, 'Previous failures must exist if no new errors are found and CTQ is still broken' );
+ winston.info( 'broken -> broken, no new errors to report' );
+ }
+ }
+ else {
+ winston.info( 'passing -> passing' );
+ }
+ }
}
module.exports = QuickServer; |
|
Before adding back in transpiling and fuzzing, I also want to do more thorough testing of lint + tsc errors, similar to what was done above, but with more variation, like a new lint and tsc error in the same cycle, two new errors of the same type in the same cycle, mixing in errors in different files, fixing errors over many cycles instead of all at once, etc to make sure all those cases are working appropriately. |
Does this mean you have to get two passing rounds before it considers the error solved? What if in a round an error is solved and another appears? Is this important? |
No, I just wanted to make sure that the state went from
With the changes above, it would just report the new error and not send a passing message. I'm considering trying out some feedback that would reflect that change, but in a way that would not send additional messages. Maybe like: If new errors show up and there are still errors that were previously reported, maybe something like this: |
|
I got close to finishing tonight but am not quite there. Latest patch: Index: js/server/QuickServer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/QuickServer.js b/js/server/QuickServer.js
--- a/js/server/QuickServer.js (revision e11b7af15b8976ab74a2db6f66a8140128fa4616)
+++ b/js/server/QuickServer.js (date 1653542733302)
@@ -26,6 +26,14 @@
const winston = require( 'winston' );
const sendSlackMessage = require( './sendSlackMessage' );
+const ctqType = {
+ LINT: 'lint',
+ TSC: 'tsc',
+ TRANSPILE: 'transpile',
+ SIM_FUZZ: 'simFuzz',
+ STUDIO_FUZZ: 'studioFuzz'
+};
+
// Headers that we'll include in all server replies
const jsonHeaders = {
'Content-Type': 'application/json',
@@ -45,7 +53,7 @@
};
// @public {*}
- this.reportState = {};
+ this.testingState = {};
// @public {string} - root of your GitHub working copy, relative to the name of the directory that the
// currently-executing script resides in
@@ -53,6 +61,9 @@
// @public {boolean} - whether we are in testing mode. if true, tests are continuously forced to run
this.isTestMode = options.isTestMode;
+
+ // @public {string[]} - errors found in any given loop
+ this.errorMessages = [];
}
/**
@@ -161,69 +172,46 @@
studioFuzz = e;
}
- const executeResultToOutput = result => {
+ const executeResultToOutput = ( result, name ) => {
return {
passed: result.code === 0,
- message: `code: ${result.code}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`
+ // TODO: CK: I changed the message to only include stdout, is this okay? Wasn't seeing how the other two were
+ // helpful in the context of Slack messages
+ errorMessages: this.parseErrors( result.stdout, name )
};
};
- const fuzzResultToOutput = result => {
+ const fuzzResultToOutput = ( result, name ) => {
if ( result === null ) {
- return { passed: true, message: '' };
+ return { passed: true, errorMessages: [] };
}
else {
- return { passed: false, message: '' + result };
+ return { passed: false, errorMessages: this.parseErrors( result, name ) };
}
};
// This would take up too much space
transpileResult.stdout = '';
- this.reportState = {
- lint: executeResultToOutput( lintResult ),
- tsc: executeResultToOutput( tscResult ),
- transpile: executeResultToOutput( transpileResult ),
- simFuzz: fuzzResultToOutput( simFuzz ),
- studioFuzz: fuzzResultToOutput( studioFuzz ),
+ this.testingState = {
+ lint: executeResultToOutput( lintResult, ctqType.LINT ),
+ tsc: executeResultToOutput( tscResult, ctqType.TSC ),
+ transpile: executeResultToOutput( transpileResult, ctqType.TRANSPILE ),
+ simFuzz: fuzzResultToOutput( simFuzz, ctqType.SIM_FUZZ ),
+ studioFuzz: fuzzResultToOutput( studioFuzz, ctqType.STUDIO_FUZZ ),
shas: shas,
timestamp: timestamp
};
try {
- const broken = !this.reportState.lint.passed ||
- !this.reportState.tsc.passed ||
- !this.reportState.transpile.passed ||
- !this.reportState.simFuzz.passed ||
- !this.reportState.studioFuzz.passed;
-
- if ( lastBroken !== broken ) {
-
- if ( broken ) {
- winston.info( 'sending broken CT message' );
- let message = 'Quick CT failing';
-
- const check = ( result, name ) => {
- if ( !result.passed ) {
- message += `\n${name}:\n\`\`\`\n${result.message}\n\`\`\`\n`;
- }
- };
+ const broken = !this.testingState.lint.passed ||
+ !this.testingState.tsc.passed ||
+ !this.testingState.transpile.passed ||
+ !this.testingState.simFuzz.passed ||
+ !this.testingState.studioFuzz.passed;
- check( this.reportState.lint, 'lint' );
- check( this.reportState.tsc, 'tsc' );
- check( this.reportState.transpile, 'transpile' );
- check( this.reportState.simFuzz, 'simFuzz' );
- check( this.reportState.studioFuzz, 'studioFuzz' );
- winston.info( message );
-
- await sendSlackMessage( message );
- }
- else {
- winston.info( 'sending passing CT message' );
- await sendSlackMessage( 'Quick CT passing' );
- }
+ await this.reportErrorStatus( broken, lastBroken );
- lastBroken = broken;
- }
+ lastBroken = broken;
}
catch( e ) {
winston.info( `Slack error: ${e}` );
@@ -253,7 +241,7 @@
if ( requestInfo.pathname === '/quickserver/status' ) {
res.writeHead( 200, jsonHeaders );
- res.end( JSON.stringify( this.reportState, null, 2 ) );
+ res.end( JSON.stringify( this.testingState, null, 2 ) );
}
}
catch( e ) {
@@ -263,6 +251,92 @@
winston.info( `QuickServer: running on port ${port}` );
}
+
+ /**
+ * Checks the error messages and reports the current status to the logs and Slack.
+ *
+ * @param {boolean} broken
+ * @param {boolean} lastBroken
+ * @private
+ */
+ async reportErrorStatus( broken, lastBroken ) {
+ if ( lastBroken !== broken && !broken ) {
+ this.errorMessages.length = 0;
+ winston.info( 'broken -> passing, sending CTQ passing message to Slack' );
+ await sendSlackMessage( 'CTQ passing', this.isTestMode );
+ }
+ else if ( broken ) {
+ let message = '';
+ let previousFailureFound = false;
+
+ const check = result => {
+ if ( !result.passed ) {
+ result.errorMessages.forEach( errorMessage => {
+ if ( !this.errorMessages.includes( errorMessage ) ) {
+ this.errorMessages.push( errorMessage );
+ message += `\n${errorMessage}`;
+ }
+ else {
+ previousFailureFound = true;
+ }
+ } );
+ }
+ };
+
+ check( this.testingState.lint );
+ check( this.testingState.tsc );
+ check( this.testingState.transpile );
+ check( this.testingState.simFuzz );
+ check( this.testingState.studioFuzz );
+
+ if ( message.length > 0 ) {
+
+ if ( previousFailureFound ) {
+ winston.info( 'broken -> more broken, sending additional CTQ failure message to Slack' );
+ message = 'CTQ additional failure:\n```' + message + '```';
+ // TODO: Report number of other, pr-existing failures
+ }
+ else {
+ winston.info( 'passing -> broken, sending CTQ failure message to Slack' );
+ message = 'CTQ failing:\n```' + message + '```';
+ }
+
+ winston.info( message );
+ await sendSlackMessage( message, this.isTestMode );
+ }
+ else {
+ assert && assert( previousFailureFound, 'Previous failures must exist if no new errors are found and CTQ is still broken' );
+ winston.info( 'broken -> broken, no new failures to report to Slack' );
+ }
+ }
+ else {
+ winston.info( 'passing -> passing' );
+ }
+ }
+
+ /**
+ * Parses individual errors out of a collection of the same type of error, e.g. lint
+ *
+ * @param {string} message
+ * @param {string} name
+ * @returns {string[]}
+ * @private
+ */
+ parseErrors( message, name ) {
+ const errorMessages = [];
+
+ // TODO: parse out individual errors from lint and tsc messages, create one line error messages per error
+ // if ( name === ctqType.LINT ) {
+ // return [];
+ // }
+ // else if ( name === ctqType.TSC ) {
+ // return [];
+ // }
+ // else {
+ errorMessages.push( `${name}: ${message}` );
+ // }
+ return errorMessages;
+ }
}
module.exports = QuickServer;
Index: js/server/sendSlackMessage.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/sendSlackMessage.js b/js/server/sendSlackMessage.js
--- a/js/server/sendSlackMessage.js (revision e11b7af15b8976ab74a2db6f66a8140128fa4616)
+++ b/js/server/sendSlackMessage.js (date 1653531229880)
@@ -12,7 +12,8 @@
let app;
-module.exports = async messageText => {
+module.exports = async ( messageText, testing = false ) => {
+
// Lazily initialized so we don't force the App creation when this file is required
if ( !app ) {
app = new App( {
@@ -21,9 +22,18 @@
} );
}
- await app.client.chat.postMessage( {
- // #continuous-testing chat ID
- channel: 'C03D6JMPAHF',
- text: messageText
- } );
+ if ( testing ) {
+ await app.client.chat.postMessage( {
+ // #ctq-testing chat ID
+ channel: 'C03G9D6NY07',
+ text: messageText
+ } );
+ }
+ else {
+ await app.client.chat.postMessage( {
+ // #continuous-testing chat ID
+ channel: 'C03D6JMPAHF',
+ text: messageText
+ } );
+ }
};
|
chrisklus
changed the title
|
I got these working in a test Slack channel, then pushed my changes. Not sure which aqua repo on bayes needs to be pulled, but I asked @jonathanolson to help me out when we return on Monday. If we like how this is working, I'll finish up cleanup + comments and close or assign for review. |
|
@pixelzoom reported a problem in #141 that would have been prevented if this code was in TypeScript, so I'll do the conversion as part of cleanup for this issue. |
|
I believe this is working as it is intended, but I thought I'd post a situation in which this change seems to not be an improvement. This morning @AgustinVallejo and I were in the middle of committing to ~20 repos and 200 files, there were merge conflicts so all in all it took about 30 minutes to get things committed. Here is what the CT slack channel said: tsc: ../../../axon/js/BooleanProperty.ts(11,29): error TS2792: Cannot find module '../../phet-core/js/types/EmptyObjectType.js'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option? CT Notifier tsc: ../../../joist/js/AboutDialog.ts(11,29): error TS2792: Cannot find module '../../phet-core/js/types/EmptyObjectType.js'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option? I am not recommending changes necessarily, but this is all one problem. If CT should remain as loud as we want it to be, how should we make it so that all of these post as a single error? Isn't that ideal? |
The CT channel has largely been noise for me and the two times there have been errors that I should fix, they have both not been reported in the channel.
Proposal: Until #140 is fixed, filter out
Error: Protocol error: Connection closed. Most likely the page has been closed.andError: Navigation failed because browser has disconnected!. Also, report all new lint and tsc failures, if possible. Not reporting a new quick-type error because a different, unrelated error already exists is not preventing noise, it's covering up something that also needs to be fixed at high priority.Marking for dev meeting in case this is not discussed before then.
The text was updated successfully, but these errors were encountered: