Closed chrisklus closed 2 years ago
@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.
chk@voyager:~/phetsims/aqua$ grunt quick-server --testing
Running "quick-server" task
info: QuickServer: running on port 45367
info: QuickServer: cycle start
info: QuickServer: checking stale // no action, letting clean test pass for first round
(node:9404) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: passing -> passing
info: QuickServer: cycle start
info: QuickServer: checking stale // added one lint error
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: passing -> broken, sending broken CT message
info: Quick CT failing:
lint:
code: 6
stdout:
Running "lint-everything" task
/Users/chk/phetsims/geometric-optics/js/geometric-optics-main.ts
15:27 error Unexpected trailing comma comma-dangle
✖ 1 problem (1 error, 0 warnings)
1 error and 0 warnings potentially fixable with the `--fix` option.
Warning: 1 errors and 0 warnings Use --force to continue.
Aborted due to warnings.
stderr:
info: QuickServer: cycle start
info: QuickServer: checking stale // added one TypeScript error
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: Deleting directory /Users/chk/phetsims//chipper/dist
info: QuickServer: tsc
info: broken -> more broken, sending additional broken CT message
info: Quick CT additional failure:
tsc:
code: 2
stdout:
../../../geometric-optics/js/geometric-optics-main.ts(17,7): error TS2339: Property 'hi' does not exist on type 'GOSim'.
stderr:
info: QuickServer: cycle start
info: QuickServer: checking stale // added a second lint error
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: broken -> more broken, sending additional broken CT message
info: Quick CT additional failure:
lint:
code: 6
stdout:
Running "lint-everything" task
/Users/chk/phetsims/geometric-optics/js/geometric-optics-main.ts
15:27 error Unexpected trailing comma comma-dangle
18:4 error Missing semicolon semi
✖ 2 problems (2 errors, 0 warnings)
2 errors and 0 warnings potentially fixable with the `--fix` option.
Warning: 2 errors and 0 warnings Use --force to continue.
Aborted due to warnings.
stderr:
info: QuickServer: cycle start
info: QuickServer: checking stale // no action, letting sit one round to see that none of the 3 reported errors are re-reported
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: broken -> broken, no new errors to report
info: QuickServer: cycle start
info: QuickServer: checking stale // added a second typescript error
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: broken -> more broken, sending additional broken CT message
info: Quick CT additional failure:
tsc:
code: 2
stdout:
../../../geometric-optics/js/geometric-optics-main.ts(14,71): error TS2339: Property 'hello' does not exist on type '{ title: string; }'.
../../../geometric-optics/js/geometric-optics-main.ts(17,7): error TS2339: Property 'hi' does not exist on type 'GOSim'.
stderr:
info: QuickServer: cycle start
info: QuickServer: checking stale // fixed all errors
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: broken -> passing, sending passing CT message
info: QuickServer: cycle start
info: QuickServer: checking stale // no action, making sure passing state is maintained
info: QuickServer: stale repos:
info: QuickServer: cloning missing repos
info: Cloning missing repos
info: QuickServer: checking SHAs
info: QuickServer: linting
info: QuickServer: tsc
info: passing -> passing
info: QuickServer: cycle start
info: QuickServer: checking stale
^C
Caught interrupt signal, exiting
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.
// no action, making sure passing state is maintained
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?
Does this mean you have to get two passing rounds before it considers the error solved?
No, I just wanted to make sure that the state went from broken -> passing
to passing -> passing
. Note that the passing CT message went out before that comment.
What if in a round an error is solved and another appears? Is this important?
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:
2 new lint errors:
*error details*
all other pre-existing errors fixed.
If new errors show up and there are still errors that were previously reported, maybe something like this:
2 new lint errors:
*error details*
3 other pre-existing errors remain.
I got close to finishing tonight but am not quite there. Latest patch:
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 https://github.com/phetsims/aqua/issues/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:
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?
Thanks @zepumph - agreed that case was not ideal. I moved this item to a list of remaining work to investigate in #152, and am going to close this issue since the refactor to report all new errors is complete.
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 https://github.com/phetsims/aqua/issues/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.