phetsims / aqua

Automatic QUality Assurance
MIT License
2 stars 4 forks source link

CT: include random seed in all CT error reports #212

Closed zepumph closed 5 months ago

zepumph commented 6 months ago

From a conversation from @samreid and @jonathanolson, this may help us reproduce things locally.

zepumph commented 6 months ago

I'm sorta stumped on how to do this. (at all/generaly/well). I'd like to touch base with @samreid again. Sorry about that.

samreid commented 6 months ago

@zepumph and I added some helpful debug info. We will notify the team. Closing.

zepumph commented 5 months ago

This change can cause an infinite loop if an assertion occurs during a strictAxonDependency check. Above I fixed that loop. This was part of the problem over in https://github.com/phetsims/faradays-electromagnetic-lab/issues/187

zepumph commented 5 months ago

@samreid can you please spot check

samreid commented 5 months ago

Looks good. Additionally I was wondering if we could make assert more agnostic about Sim.ts like in this patch:

```diff Subject: [PATCH] Move HasChangedNumberProperty to a separate file, see https://github.com/phetsims/density-buoyancy-common/issues/123 --- Index: chipper/phet-types.d.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/phet-types.d.ts b/chipper/phet-types.d.ts --- a/chipper/phet-types.d.ts (revision 739e543ddef46bcda67e058ed0e0e2a422142fbd) +++ b/chipper/phet-types.d.ts (date 1718124959961) @@ -121,6 +121,7 @@ declare var assertions: { enableAssert: () => void; + assertionHooks: Array<() => void>; }; // Experiment to allow accessing these off window. See https://stackoverflow.com/questions/12709074/how-do-you-explicitly-set-a-new-property-on-window-in-typescript Index: assert/js/assert.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/assert/js/assert.js b/assert/js/assert.js --- a/assert/js/assert.js (revision 2abb8a3116463c20d4e32551bc6da2de33601bf2) +++ b/assert/js/assert.js (date 1718125019408) @@ -6,12 +6,12 @@ ( function() { - window.assertions = window.assertions || {}; + window.assertions.assertionHooks = []; window.assertions.assertFunction = window.assertions.assertFunction || function( predicate, ...messages ) { if ( !predicate ) { - // don't treat falsey as a message. + // don't treat falsy as a message. messages = messages.filter( message => !!messages ); // Log the stack trace to IE. Just creating an Error is not enough, it has to be caught to get a stack. @@ -23,8 +23,7 @@ const assertPrefix = messages.length > 0 ? 'Assertion failed: ' : 'Assertion failed'; console && console.error && console.error( assertPrefix, ...messages ); - // eslint-disable-next-line bad-phet-library-text - window.phet?.joist?.sim && console && console.log && console.log( 'Debug info:', JSON.stringify( window.phet.joist.sim.getAssertionDebugInfo(), null, 2 ) ); + window.assertions.assertionHooks.forEach( hook => hook() ); if ( window.QueryStringMachine && QueryStringMachine.containsKey( 'debugger' ) ) { debugger; // eslint-disable-line no-debugger Index: joist/js/Sim.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/Sim.ts b/joist/js/Sim.ts --- a/joist/js/Sim.ts (revision ff8657b06963957a9cd45db83fcb83690268c19e) +++ b/joist/js/Sim.ts (date 1718124959973) @@ -266,6 +266,11 @@ assert && assert( allSimScreens.length >= 1, 'at least one screen is required' ); + // If an assertion fails while a Sim exists, add some helpful information about the context of the failure + window.assertions.assertionHooks.push( () => { + console.log( 'Debug info:', JSON.stringify( this.getAssertionDebugInfo(), null, 2 ) ); + } ); + const options = optionize()( { credits: {},
zepumph commented 5 months ago

I love it. Thanks so much for the idea.