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

Unit tests for code quality and grunt tasks and task running #384

Open
samreid opened this issue Oct 23, 2024 · 7 comments
Open

Unit tests for code quality and grunt tasks and task running #384

samreid opened this issue Oct 23, 2024 · 7 comments

Comments

@samreid
Copy link
Member

samreid commented Oct 23, 2024

We should add unit tests that:

  • Ensure things like sage run myScript.ts continue to operate correctly.
  • Add a temporary synthetic type error like const m: number = 'seven'; and make sure grunt check fails
  • Add a temporary lint error like 3 blank lines and make sure grunt lint fails.

CT should support node-based QUnit. We should test this on CT and local running. May not need to be wired in to precommit hooks. Maybe precommit hooks? I'm concerned about mutating files though.

@zepumph
Copy link
Member

zepumph commented Oct 24, 2024

perennial and chipper both support command line qunit tests with npm run test. We should get these running on ct, and feel free to add to them. Please note that these tests should not have side effects in order to run on CT. For example, perennialGruntTest.js checks out shas for many repos, and that will not work well for tests running on CT. We should be careful about what we include in "standard" unit tests.

@zepumph
Copy link
Member

zepumph commented Oct 24, 2024

Should we move to a single qunit package in perennial, related to #372. Then chipper's test command would be:

../perennial-alias/node_module/qunit/ or something like that.

@zepumph zepumph self-assigned this Oct 24, 2024
@zepumph
Copy link
Member

zepumph commented Oct 24, 2024

After doing some experimenting. I believe I am ready to explore jest for our command line unit tests. Concerns:

  1. A way to easily opt out of always running perennialGruntTest, which makes one offs, rcs, and production deploys for bumber.
  2. We want testing files to be in TypeScript. I am blocked by this because I want to use a dependency that is in typescript. And I cannot import ts from js.

@zepumph
Copy link
Member

zepumph commented Oct 24, 2024

Jest doesn't seem like an obvious win. I'm unsure how best to support typescript. Here is a patch that fixes SimVersionTests to support jest. Unknowns:

  • Supporting test files as typescript
  • lint/tsc working for jest globals
Subject: [PATCH] use bumper for all tests, https://github.com/phetsims/perennial/issues/384
---
Index: js/common/SimVersionTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/SimVersionTests.js b/js/common/SimVersionTests.js
--- a/js/common/SimVersionTests.js	(revision 06056f4fb5bd065ae2a9c4dc3f41e1d070c8d608)
+++ b/js/common/SimVersionTests.js	(date 1729804053194)
@@ -6,18 +6,15 @@
  */
 
 const SimVersion = require( './SimVersion' );
-const qunit = require( 'qunit' );
 
-qunit.module( 'SimVersion' );
 
-
-qunit.test( 'SimVersion Basics', async assert => {
+test( 'SimVersion Basics', () => {
 
   const testVersion = ( simVersion, major, minor, maintenance, message ) => {
 
-    assert.ok( simVersion.major === major, `major: ${message}` );
-    assert.ok( simVersion.minor === minor, `minor: ${message}` );
-    assert.ok( simVersion.maintenance === maintenance, `maintenance: ${message}` );
+    expect( simVersion.major === major );
+    expect( simVersion.minor === minor );
+    expect( simVersion.maintenance === maintenance );
   };
 
   const simVersion = new SimVersion( 1, 2, 0 );
@@ -46,24 +43,23 @@
   testVersion( versions[ 1 ], 2, 2, 2, 'another sorted second' );
   testVersion( versions[ 2 ], 3, 0, 0, 'another sorted third' );
 
-  assert.throws( () => {
-    return SimVersion( '1fdsaf', '2fdsaf', '3fdsa' );
-  }, 'letters as version, boo' );
+  expect( () => {
+    return new SimVersion( '1fdsaf', '2fdsaf', '3fdsa' );
+  } ).toThrow( 'major version should be a non-negative integer' );
 
-  assert.throws( () => {
-    return SimVersion( 'fdsaf1fdsaf', 'fdsaf2fdsaf', 'fdsa3fdsa' );
-  }, 'letters as version, boo two' );
+  expect( () => {
+    return new SimVersion( 1, 'fdsaf2fdsaf', 'fdsa3fdsa' );
+  } ).toThrow( 'minor version should be a non-negative integer' );
 
-  assert.throws( () => {
-    return SimVersion( true, false, true );
-  }, 'letters as version, boo' );
+  expect( () => {
+    return new SimVersion( true, false, true );
+  } ).toThrow( 'major version should be a non-negative integer' );
 
   const mySimVersion = new SimVersion( '1', '2', '3', {
     testType: 'rc',
     testNumber: '1'
   } );
-  testVersion( mySimVersion, 1, 2, 3, 'basic constructor' );
-  assert.ok( mySimVersion.testNumber === 1, 'testNumber number cast check' );
-  assert.ok( mySimVersion.toString() === '1.2.3-rc.1', 'as string' );
-
+  testVersion( mySimVersion, 1, 2, 3 );
+  expect( mySimVersion.testNumber === 1 );
+  expect( mySimVersion.toString() === '1.2.3-rc.1' );
 } );
\ No newline at end of file
Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/package.json b/package.json
--- a/package.json	(revision 06056f4fb5bd065ae2a9c4dc3f41e1d070c8d608)
+++ b/package.json	(date 1729804090499)
@@ -7,7 +7,7 @@
     "url": "https://github.com/phetsims/perennial.git"
   },
   "scripts": {
-    "test": "qunit"
+    "test": "jest --testPathPattern=test --testPathIgnorePatterns=perennialGruntTest"
   },
   "devDependencies": {
     "@octokit/rest": "~16.33.1",
@@ -40,6 +40,7 @@
     "graceful-fs": "~4.1.11",
     "grunt": "~1.5.3",
     "html-differ": "~1.4.0",
+    "jest": "~29.7.0",
     "lodash": "~4.17.10",
     "mimelib": "~0.2.19",
     "minimist": "~1.2.8",

zepumph added a commit that referenced this issue Oct 24, 2024
zepumph added a commit that referenced this issue Oct 24, 2024
zepumph added a commit that referenced this issue Oct 24, 2024
samreid added a commit to phetsims/aqua that referenced this issue Oct 25, 2024
samreid added a commit to phetsims/phet-info that referenced this issue Oct 25, 2024
samreid added a commit to phetsims/chipper that referenced this issue Oct 25, 2024
@samreid
Copy link
Member Author

samreid commented Nov 2, 2024

In #386 I demonstrated how to run typescript code in the qunit harness.

zepumph added a commit that referenced this issue Nov 4, 2024
@zepumph
Copy link
Member

zepumph commented Nov 4, 2024

Excellent! I like you way a lot. We should do the same for the entry point in chipper (use sage run)

zepumph added a commit that referenced this issue Nov 12, 2024
zepumph added a commit that referenced this issue Nov 12, 2024
zepumph added a commit that referenced this issue Nov 12, 2024
@zepumph
Copy link
Member

zepumph commented Nov 13, 2024

This is feeling in much better shape now. I am happy with the ability to add and run new tests, and once phetsims/aqua#223 is complete, we will be able to have CT running things as well. I would like to check in with @samreid to see if there is anything else to do here.

For example, should npm run test be a contender for pre-commit hooks, if available?

@zepumph zepumph removed their assignment Nov 13, 2024
zepumph added a commit to phetsims/chipper that referenced this issue Nov 14, 2024
zepumph added a commit that referenced this issue Nov 14, 2024
zepumph added a commit to phetsims/chipper that referenced this issue Nov 14, 2024
zepumph added a commit to phetsims/chipper that referenced this issue Nov 14, 2024
zepumph added a commit that referenced this issue Nov 14, 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