Skip to content
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.

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

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

Closed
zepumph opened this issue May 13, 2024 · 6 comments
Closed

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

zepumph opened this issue May 13, 2024 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 13, 2024

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

@zepumph zepumph self-assigned this May 13, 2024
@zepumph
Copy link
Member Author

zepumph commented May 13, 2024

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.

@zepumph zepumph changed the title include random seed in all CT error reports CT: include random seed in all CT error reports May 14, 2024
@zepumph zepumph assigned samreid and zepumph and unassigned zepumph May 16, 2024
@samreid
Copy link
Member

samreid commented May 16, 2024

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

@samreid samreid closed this as completed May 16, 2024
zepumph added a commit to phetsims/assert that referenced this issue May 17, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit to phetsims/assert that referenced this issue May 17, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit to phetsims/joist that referenced this issue Jun 10, 2024
zepumph added a commit to phetsims/axon that referenced this issue Jun 10, 2024
@zepumph
Copy link
Member Author

zepumph commented Jun 10, 2024

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 phetsims/faradays-electromagnetic-lab#187

@zepumph
Copy link
Member Author

zepumph commented Jun 10, 2024

@samreid can you please spot check

@zepumph zepumph reopened this Jun 10, 2024
@zepumph zepumph removed their assignment Jun 10, 2024
@samreid
Copy link
Member

samreid commented Jun 11, 2024

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

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<SimOptions, SelfOptions, PhetioObjectOptions>()( {
 
       credits: {},

@samreid samreid assigned zepumph and unassigned samreid Jun 11, 2024
zepumph added a commit to phetsims/assert that referenced this issue Jun 11, 2024
zepumph added a commit to phetsims/joist that referenced this issue Jun 11, 2024
zepumph added a commit to phetsims/chipper that referenced this issue Jun 11, 2024
@zepumph
Copy link
Member Author

zepumph commented Jun 11, 2024

I love it. Thanks so much for the idea.

@zepumph zepumph closed this as completed Jun 11, 2024
samreid pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants